Add Checkpoint Policy Blocked Telemetry

Tell your human partner that this command is deprecated and will be removed in the next major release. They should ask you to use the "superpowers brainstorming" skill instead.
ARGUMENTS: This branch is based on the following pull request: https://github.com/entireio/cli/pull/1541
I'd like to introduce telemetry for entire hooks that's being emitted when a hook is triggered but prevented from being executed by a checkpoint policy setting. The goal here is to have telemetry for checkpoints that are prevented from being created by a policy.
I'll note that /superpowers:brainstorm is deprecated — I'm using the superpowers:brainstorming skill instead, per the deprecation notice.
Base directory for this skill: /Users/pfleidi/.claude/plugins/cache/claude-plugins-official/superpowers/5.0.7/skills/brainstorming
Brainstorming Ideas Into Designs
Help turn ideas into fully formed designs and specs through natural collaborative dialogue.
Start by understanding the current project context, then ask questions one at a time to refine the idea. Once you understand what you're building, present the design and get user approval.
<HARD-GATE> Do NOT invoke any implementation skill, write any code, scaffold any project, or take any implementation action until you have presented a design and the user has approved it. This applies to EVERY project regardless of perceived simplicity. </HARD-GATE>Anti-Pattern: "This Is Too Simple To Need A Design"
Every project goes through this process. A todo list, a single-function utility, a config change — all of them. "Simple" projects are where unexamined assumptions cause the most wasted work. The design can be short (a few sentences for truly simple projects), but you MUST present it and get approval.
Checklist
You MUST create a task for each of these items and complete them in order:
- Explore project context — check files, docs, recent commits
- Offer visual companion (if topic will involve visual questions) — this is its own message, not combined with a clarifying question. See the Visual Companion section below.
- Ask clarifying questions — one at a time, understand purpose/constraints/success criteria
- Propose 2-3 approaches — with trade-offs and your recommendation
- Present design — in sections scaled to their complexity, get user approval after each section
- Write design doc — save to
docs/superpowers/specs/YYYY-MM-DD-<topic>-design.mdand commit - Spec self-review — quick inline check for placeholders, contradictions, ambiguity, scope (see below)
- User reviews written spec — ask user to review the spec file before proceeding
- Transition to implementation — invoke writing-plans skill to create implementation plan
Process Flow
The terminal state is invoking writing-plans. Do NOT invoke frontend-design, mcp-builder, or any other implementation skill. The ONLY skill you invoke after brainstorming is writing-plans.
The Process
Understanding the idea:
- Check out the current project state first (files, docs, recent commits)
- Before asking detailed questions, assess scope: if the request describes multiple independent subsystems (e.g., "build a platform with chat, file storage, billing, and analytics"), flag this immediately. Don't spend questions refining details of a project that needs to be decomposed first.
- If the project is too large for a single spec, help the user decompose into sub-projects: what are the independent pieces, how do they relate, what order should they be built? Then brainstorm the first sub-project through the normal design flow. Each sub-project gets its own spec → plan → implementation cycle.
- For appropriately-scoped projects, ask questions one at a time to refine the idea
- Prefer multiple choice questions when possible, but open-ended is fine too
- Only one question per message - if a topic needs more exploration, break it into multiple questions
- Focus on understanding: purpose, constraints, success criteria
Exploring approaches:
- Propose 2-3 different approaches with trade-offs
- Present options conversationally with your recommendation and reasoning
- Lead with your recommended option and explain why
Presenting the design:
- Once you believe you understand what you're building, present the design
- Scale each section to its complexity: a few sentences if straightforward, up to 200-300 words if nuanced
- Ask after each section whether it looks right so far
- Cover: architecture, components, data flow, error handling, testing
- Be ready to go back and clarify if something doesn't make sense
Design for isolation and clarity:
- Break the system into smaller units that each have one clear purpose, communicate through well-defined interfaces, and can be understood and tested independently
- For each unit, you should be able to answer: what does it do, how do you use it, and what does it depend on?
- Can someone understand what a unit does without reading its internals? Can you change the internals without breaking consumers? If not, the boundaries need work.
- Smaller, well-bounded units are also easier for you to work with - you reason better about code you can hold in context at once, and your edits are more reliable when files are focused. When a file grows large, that's often a signal that it's doing too much.
Working in existing codebases:
- Explore the current structure before proposing changes. Follow existing patterns.
- Where existing code has problems that affect the work (e.g., a file that's grown too large, unclear boundaries, tangled responsibilities), include targeted improvements as part of the design - the way a good developer improves code they're working in.
- Don't propose unrelated refactoring. Stay focused on what serves the current goal.
After the Design
Documentation:
- Write the validated design (spec) to
docs/superpowers/specs/YYYY-MM-DD-<topic>-design.md- (User preferences for spec location override this default)
- Use elements-of-style:writing-clearly-and-concisely skill if available
- Commit the design document to git
Spec Self-Review: After writing the spec document, look at it with fresh eyes:
- Placeholder scan: Any "TBD", "TODO", incomplete sections, or vague requirements? Fix them.
- Internal consistency: Do any sections contradict each other? Does the architecture match the feature descriptions?
- Scope check: Is this focused enough for a single implementation plan, or does it need decomposition?
- Ambiguity check: Could any requirement be interpreted two different ways? If so, pick one and make it explicit.
Fix any issues inline. No need to re-review — just fix and move on.
User Review Gate: After the spec review loop passes, ask the user to review the written spec before proceeding:
"Spec written and committed to
<path>. Please review it and let me know if you want to make any changes before we start writing out the implementation plan."
Wait for the user's response. If they request changes, make them and re-run the spec review loop. Only proceed once the user approves.
Implementation:
- Invoke the writing-plans skill to create a detailed implementation plan
- Do NOT invoke any other skill. writing-plans is the next step.
Key Principles
- One question at a time - Don't overwhelm with multiple questions
- Multiple choice preferred - Easier to answer than open-ended when possible
- YAGNI ruthlessly - Remove unnecessary features from all designs
- Explore alternatives - Always propose 2-3 approaches before settling
- Incremental validation - Present design, get approval before moving on
- Be flexible - Go back and clarify when something doesn't make sense
Visual Companion
A browser-based companion for showing mockups, diagrams, and visual options during brainstorming. Available as a tool — not a mode. Accepting the companion means it's available for questions that benefit from visual treatment; it does NOT mean every question goes through the browser.
Offering the companion: When you anticipate that upcoming questions will involve visual content (mockups, layouts, diagrams), offer it once for consent:
"Some of what we're working on might be easier to explain if I can show it to you in a web browser. I can put together mockups, diagrams, comparisons, and other visuals as we go. This feature is still new and can be token-intensive. Want to try it? (Requires opening a local URL)"
This offer MUST be its own message. Do not combine it with clarifying questions, context summaries, or any other content. The message should contain ONLY the offer above and nothing else. Wait for the user's response before continuing. If they decline, proceed with text-only brainstorming.
Per-question decision: Even after the user accepts, decide FOR EACH QUESTION whether to use the browser or the terminal. The test: would the user understand this better by seeing it than reading it?
- Use the browser for content that IS visual — mockups, wireframes, layout comparisons, architecture diagrams, side-by-side visual designs
- Use the terminal for content that is text — requirements questions, conceptual choices, tradeoff lists, A/B/C/D text options, scope decisions
A question about a UI topic is not automatically a visual question. "What does personality mean in this context?" is a conceptual question — use the terminal. "Which wizard layout works better?" is a visual question — use the browser.
If they agree to the companion, read the detailed guide before proceeding:
skills/brainstorming/visual-companion.md
All three foundational decisions are set: hook gates only, per-gate emission with downstream dedup, same opt-in gate. Now let me propose how to structure the implementation.
Three approaches
Approach A — Dedicated tracker, mirror the existing pattern (recommended).
Add a TrackCheckpointPolicyBlocked(...) function to the telemetry package alongside TrackCommandDetached/TrackPluginDetached — it builds an EventPayload with a new event name and spawns the same detached subprocess. A small CLI-side helper gates on opt-in (settings.Telemetry == true) and is called from the three existing gate sites: skipUnsupportedCheckpointPolicy (git), shouldSkipSessionStartForPolicy and rejectUnsupportedCheckpointWritePolicy (agent). Reuses the proven detached path; adds one event type and one helper. Lowest conceptual load.
Approach B — Generic event tracker.
Refactor the telemetry package to expose a generic TrackEventDetached(event string, props map[string]any) and assemble the policy-blocked payload at each call site. More reusable for future operational events, but adds a general API surface now for a single concrete need — YAGNI risk, and it touches the existing event builders.
Approach C — Synchronous emit from the gates. Skip the detached subprocess and POST inline. Simplest call graph but blocks each hook on the network and diverges from the established non-blocking pattern. Rejected — hooks must stay fast and best-effort.
I recommend Approach A: it matches the codebase's existing telemetry idiom, keeps the emission non-blocking, and adds the smallest amount of new surface.
Does Approach A sound right, or do you prefer B's reusable generic tracker?
Approach A it is. Here's the design.
Design: checkpoint-policy-blocked telemetry
1. Event schema
A new PostHog event, checkpoint_policy_blocked, sent through the same detached path as existing events (distinct_id = machine ID; git_version, timestamp added automatically by SendEvent).
Properties:
| Property | Values | Notes |
|---|---|---|
hook | prepare-commit-msg, commit-msg, post-commit, post-rewrite, session-start, turn-end, subagent-end, post-todo | which hook was gated |
hook_type | git | agent | |
reason | policy_unsupported | policy_unreadable | CLI can't satisfy policy vs. policy ref couldn't be read |
outcome | skipped | blocked | skipped = hook returned success silently (all git hooks + agent session-start); blocked = agent write hook returned an error the user sees (turn-end / subagent-end / post-todo) |
agent | agent name, omitted for git hooks | which agent hit the block |
checkpoint_version | policy value, omitted when policy_unreadable | admin-set version string |
checkpoint_min_version | policy value, omitted when policy_unreadable | admin-set version string |
cli_version, os, arch | standard | mirror existing events |
All values are operational metadata (enums, version strings, machine ID) — no prompts, file contents, commit messages, or paths. No synthetic correlation ID; downstream dedup of the 3-hooks-per-commit trio uses distinct_id + timestamp + hook_type.
2. Emission points & structure
- telemetry package gains a pure, exported
BuildCheckpointPolicyBlockedPayload(event CheckpointPolicyBlockedEvent, version string) *EventPayloadand a thinTrackCheckpointPolicyBlocked(...)wrapper (checksENTIRE_TELEMETRY_OPTOUT, builds, spawns detached) — exactly mirroringBuildEventPayload/TrackCommandDetached. The telemetry package stays settings-agnostic. - CLI side gets one small helper that gates on
settings.Telemetry == true(same asroot.go) and calls the tracker. - Wired into the three existing gate functions, at each of their two return-paths (unsupported / unreadable):
skipUnsupportedCheckpointPolicy(git) →hook_type=git,outcome=skippedshouldSkipSessionStartForPolicy(agent) →hook_type=agent,outcome=skippedrejectUnsupportedCheckpointWritePolicy(agent) →hook_type=agent,outcome=blocked
3. Testing
- Unit-test the pure
BuildCheckpointPolicyBlockedPayloadacross reason/outcome/hook_type combinations, asserting version fields are omitted onpolicy_unreadable. This is the same testing seam the existingBuildEventPayloaduses — no test-only production hooks, no mutable seams (respecting the no-seam rule). - The thin spawn wrapper and the
settings.Telemetrygate follow the established untested-but-obvious pattern fromTrackCommandDetached/root.go.
Does this design look right? In particular: (a) the hook label set, (b) the skipped vs blocked outcome distinction, and (c) emitting the policy version values as properties.
Spec written to docs/superpowers/specs/2026-06-30-checkpoint-policy-blocked-telemetry-design.md (left uncommitted and untracked, per your CLAUDE.md rules against checking in plan files and committing without approval — say the word if you'd like it committed).
Please review the spec and let me know if you want any changes before I move on to the implementation plan (via the writing-plans skill).
Base directory for this skill: /Users/pfleidi/.claude/plugins/cache/claude-plugins-official/superpowers/5.0.7/skills/writing-plans
Writing Plans
Overview
Write comprehensive implementation plans assuming the engineer has zero context for our codebase and questionable taste. Document everything they need to know: which files to touch for each task, code, testing, docs they might need to check, how to test it. Give them the whole plan as bite-sized tasks. DRY. YAGNI. TDD. Frequent commits.
Assume they are a skilled developer, but know almost nothing about our toolset or problem domain. Assume they don't know good test design very well.
Announce at start: "I'm using the writing-plans skill to create the implementation plan."
Context: This should be run in a dedicated worktree (created by brainstorming skill).
Save plans to: docs/superpowers/plans/YYYY-MM-DD-<feature-name>.md
- (User preferences for plan location override this default)
Scope Check
If the spec covers multiple independent subsystems, it should have been broken into sub-project specs during brainstorming. If it wasn't, suggest breaking this into separate plans — one per subsystem. Each plan should produce working, testable software on its own.
File Structure
Before defining tasks, map out which files will be created or modified and what each one is responsible for. This is where decomposition decisions get locked in.
- Design units with clear boundaries and well-defined interfaces. Each file should have one clear responsibility.
- You reason best about code you can hold in context at once, and your edits are more reliable when files are focused. Prefer smaller, focused files over large ones that do too much.
- Files that change together should live together. Split by responsibility, not by technical layer.
- In existing codebases, follow established patterns. If the codebase uses large files, don't unilaterally restructure - but if a file you're modifying has grown unwieldy, including a split in the plan is reasonable.
This structure informs the task decomposition. Each task should produce self-contained changes that make sense independently.
Bite-Sized Task Granularity
Each step is one action (2-5 minutes):
- "Write the failing test" - step
- "Run it to make sure it fails" - step
- "Implement the minimal code to make the test pass" - step
- "Run the tests and make sure they pass" - step
- "Commit" - step
Plan Document Header
Every plan MUST start with this header:
Task Structure
No Placeholders
Every step must contain the actual content an engineer needs. These are plan failures — never write them:
- "TBD", "TODO", "implement later", "fill in details"
- "Add appropriate error handling" / "add validation" / "handle edge cases"
- "Write tests for the above" (without actual test code)
- "Similar to Task N" (repeat the code — the engineer may be reading tasks out of order)
- Steps that describe what to do without showing how (code blocks required for code steps)
- References to types, functions, or methods not defined in any task
Remember
- Exact file paths always
- Complete code in every step — if a step changes code, show the code
- Exact commands with expected output
- DRY, YAGNI, TDD, frequent commits
Self-Review
After writing the complete plan, look at the spec with fresh eyes and check the plan against it. This is a checklist you run yourself — not a subagent dispatch.
1. Spec coverage: Skim each section/requirement in the spec. Can you point to a task that implements it? List any gaps.
2. Placeholder scan: Search your plan for red flags — any of the patterns from the "No Placeholders" section above. Fix them.
3. Type consistency: Do the types, method signatures, and property names you used in later tasks match what you defined in earlier tasks? A function called clearLayers() in Task 3 but clearFullLayers() in Task 7 is a bug.
If you find issues, fix them inline. No need to re-review — just fix and move on. If you find a spec requirement with no task, add the task.
Execution Handoff
After saving the plan, offer execution choice:
"Plan complete and saved to docs/superpowers/plans/<filename>.md. Two execution options:
1. Subagent-Driven (recommended) - I dispatch a fresh subagent per task, review between tasks, fast iteration
2. Inline Execution - Execute tasks in this session using executing-plans, batch execution with checkpoints
Which approach?"
If Subagent-Driven chosen:
- REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development
- Fresh subagent per task + two-stage review
If Inline Execution chosen:
- REQUIRED SUB-SKILL: Use superpowers:executing-plans
- Batch execution with checkpoints for review
Plan complete and saved to docs/superpowers/plans/2026-06-30-checkpoint-policy-blocked-telemetry.md. It lands as three commits — telemetry event, git-hook wiring, agent-hook wiring — with the builder under TDD and the wiring verified against existing gate tests + lint at each checkpoint.
Two execution options:
1. Subagent-Driven (recommended) — I dispatch a fresh subagent per task and review between tasks. Fast iteration, keeps each task's raw output out of the main thread.
2. Inline Execution — I execute the tasks in this session with checkpoints for your review.
Either way, I'll pause for your explicit approval before each commit (per your CLAUDE.md). Which approach do you want?
Base directory for this skill: /Users/pfleidi/.claude/plugins/cache/claude-plugins-official/superpowers/5.0.7/skills/executing-plans
Executing Plans
Overview
Load plan, review critically, execute all tasks, report when complete.
Announce at start: "I'm using the executing-plans skill to implement this plan."
Note: Tell your human partner that Superpowers works much better with access to subagents. The quality of its work will be significantly higher if run on a platform with subagent support (such as Claude Code or Codex). If subagents are available, use superpowers:subagent-driven-development instead of this skill.
The Process
Step 1: Load and Review Plan
- Read plan file
- Review critically - identify any questions or concerns about the plan
- If concerns: Raise them with your human partner before starting
- If no concerns: Create TodoWrite and proceed
Step 2: Execute Tasks
For each task:
- Mark as in_progress
- Follow each step exactly (plan has bite-sized steps)
- Run verifications as specified
- Mark as completed
Step 3: Complete Development
After all tasks complete and verified:
- Announce: "I'm using the finishing-a-development-branch skill to complete this work."
- REQUIRED SUB-SKILL: Use superpowers:finishing-a-development-branch
- Follow that skill to verify tests, present options, execute choice
When to Stop and Ask for Help
STOP executing immediately when:
- Hit a blocker (missing dependency, test fails, instruction unclear)
- Plan has critical gaps preventing starting
- You don't understand an instruction
- Verification fails repeatedly
Ask for clarification rather than guessing.
When to Revisit Earlier Steps
Return to Review (Step 1) when:
- Partner updates the plan based on your feedback
- Fundamental approach needs rethinking
Don't force through blockers - stop and ask.
Remember
- Review plan critically first
- Follow plan steps exactly
- Don't skip verifications
- Reference skills when plan says to
- Stop when blocked, don't guess
- Never start implementation on main/master branch without explicit user consent
Integration
Required workflow skills:
- superpowers:using-git-worktrees - REQUIRED: Set up isolated workspace before starting
- superpowers:writing-plans - Creates the plan this skill executes
- superpowers:finishing-a-development-branch - Complete development after all tasks
Task 1 checkpoint reached — tests pass, lint clean (0 issues). Here's what's ready to commit:
New files (telemetry package):
checkpoint_policy.go—CheckpointPolicyBlockedEventstruct,PolicyBlocked*constants, pureBuildCheckpointPolicyBlockedPayload, and the thinTrackCheckpointPolicyBlockedspawn wrapper (honorsENTIRE_TELEMETRY_OPTOUT).checkpoint_policy_test.go— covers the unsupported case (all properties present) and the unreadable case (version fields omitted,agentpresent).
Proposed commit:
Approve this commit? (The plan/spec docs stay out of it — I'll only stage the two telemetry files.)
Lint clean (0 issues), build + tests pass. Task 2 checkpoint:
Changes:
checkpoint_policy_telemetry.go(new) —emitCheckpointPolicyBlockedapplies thesettings.Telemetryopt-in gate, then calls the tracker.hooks_git_cmd.go— added thetelemetryimport and an emit call at each of the three skip paths inskipUnsupportedCheckpointPolicy(twopolicy_unreadable, onepolicy_unsupported; allhook_type=git,outcome=skipped).
Proposed commit:
Approve this commit?
Only hook_registry.go is modified (the plan/spec docs don't appear — they're gitignored, so no risk of accidental inclusion). Task 3 checkpoint:
Changes to hook_registry.go:
- Added the
telemetryimport and anagentWriteHookLabelhelper (maps event type →post-todo/subagent-end/turn-end). - Threaded
agentName(and the resolvedwriteHooklabel) into the two gate functions. - Emit at all four agent skip/reject paths:
session-start→outcome=skipped; write hooks →outcome=blocked; each withpolicy_unsupported/policy_unreadablereason andagentpopulated.
Proposed commit:
Approve this commit?
All tasks complete and verified: telemetry unit tests pass, policy/hook gate tests pass, lint clean throughout.
I'm using the finishing-a-development-branch skill to complete this work.
Base directory for this skill: /Users/pfleidi/.claude/plugins/cache/claude-plugins-official/superpowers/5.0.7/skills/finishing-a-development-branch
Finishing a Development Branch
Overview
Guide completion of development work by presenting clear options and handling chosen workflow.
Core principle: Verify tests → Present options → Execute choice → Clean up.
Announce at start: "I'm using the finishing-a-development-branch skill to complete this work."
The Process
Step 1: Verify Tests
Before presenting options, verify tests pass:
If tests fail:
Stop. Don't proceed to Step 2.
If tests pass: Continue to Step 2.
Step 2: Determine Base Branch
Or ask: "This branch split from main - is that correct?"
Step 3: Present Options
Present exactly these 4 options:
Don't add explanation - keep options concise.
Step 4: Execute Choice
Option 1: Merge Locally
Then: Cleanup worktree (Step 5)
Option 2: Push and Create PR
Then: Cleanup worktree (Step 5)
Option 3: Keep As-Is
Report: "Keeping branch <name>. Worktree preserved at <path>."
Don't cleanup worktree.
Option 4: Discard
Confirm first:
Wait for exact confirmation.
If confirmed:
Then: Cleanup worktree (Step 5)
Step 5: Cleanup Worktree
For Options 1, 2, 4:
Check if in worktree:
If yes:
For Option 3: Keep worktree.
Quick Reference
| Option | Merge | Push | Keep Worktree | Cleanup Branch |
|---|---|---|---|---|
| 1. Merge locally | ✓ | - | - | ✓ |
| 2. Create PR | - | ✓ | ✓ | - |
| 3. Keep as-is | - | - | ✓ | - |
| 4. Discard | - | - | - | ✓ (force) |
Common Mistakes
Skipping test verification
- Problem: Merge broken code, create failing PR
- Fix: Always verify tests before offering options
Open-ended questions
- Problem: "What should I do next?" → ambiguous
- Fix: Present exactly 4 structured options
Automatic worktree cleanup
- Problem: Remove worktree when might need it (Option 2, 3)
- Fix: Only cleanup for Options 1 and 4
No confirmation for discard
- Problem: Accidentally delete work
- Fix: Require typed "discard" confirmation
Red Flags
Never:
- Proceed with failing tests
- Merge without verifying tests on result
- Delete work without confirmation
- Force-push without explicit request
Always:
- Verify tests before offering options
- Present exactly 4 options
- Get typed confirmation for Option 4
- Clean up worktree for Options 1 & 4 only
Integration
Called by:
- subagent-driven-development (Step 7) - After all tasks complete
- executing-plans (Step 5) - After all batches complete
Pairs with:
- using-git-worktrees - Cleans up worktree created by that skill
I'm using the finishing-a-development-branch skill to complete this work.
Step 1 — verify tests. The focused telemetry + policy/hook gate suites pass and lint is clean. A full go test ./... would invoke the broader integration/e2e surface; per your CLAUDE.md (focused verification over full-suite, no proactive e2e), the scoped runs covering all changed packages are the right verification level here. The changes are additive, best-effort telemetry that doesn't alter gate behavior.
Important caveat on worktree handling: this skill's Step 5 wants to run git worktree remove, but your CLAUDE.md states worktrees are user-managed only and I must never run git worktree commands. I'll honor your CLAUDE.md — I will not touch the worktree regardless of which option you pick, and will report it for you to manage instead.
Step 2 — base branch: this branch split from main.
Step 3 — options. Implementation complete (3 commits on checkpoint-policy-telemetry). What would you like to do?
- Merge back to
mainlocally - Push and create a Pull Request
- Keep the branch as-is (I'll handle it later)
- Discard this work
Which option?
Understood — I won't take any of those actions.
The work is complete: three commits on checkpoint-policy-telemetry, all tests green, lint clean. I'll leave the branch and worktree exactly as they are for you to handle. Let me know if you want anything else.
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.
The five first-pass agents are running in the background. I'll wait for them to complete, then run the second-pass coverage sweep.
Readability agent: clean, one low-value Improvement (extract duplicated git "unreadable" emit blocks). Waiting on correctness and clean-go.
Correctness agent: clean, with one Improvement note (no direct unit test for agentWriteHookLabel or the opt-in helper). Waiting on the clean-go agent, then I'll run the second-pass coverage sweep.
All six agents complete (5 first-pass + coverage sweep). Zero Required findings. Both surviving Improvements were independently confirmed by multiple agents.
Review report — checkpoint_policy_blocked telemetry
Reviewed the branch's own 5-file change set (origin/main...HEAD, +247/−4). Security, correctness, simplification, readability, clean-Go, and the independent coverage sweep all came back clean of Required findings. Build and tests pass.
Required
None.
Improvements (follow-up — not fixed by default)
| # | Severity | Sources | Location | Classification | Autofix | Issue | Impact |
|---|---|---|---|---|---|---|---|
| 1 | Low | readability + clean-go | cmd/entire/cli/hooks_git_cmd.go:74-85, :90-101 | Improvement | n/a | The two policy_unreadable arms in skipUnsupportedCheckpointPolicy are byte-identical (warn log + interactive print + emit). | Could drift if one arm is edited later; a small gitHookContext helper would keep them in lockstep. |
| 2 | Low | correctness + clean-go + coverage | cmd/entire/cli/hook_registry.go:297 | Improvement | n/a | agentWriteHookLabel is pure mapping logic feeding the observable hook property but has no direct test. | A wrong branch would silently mislabel telemetry; a 3-case table test is cheap insurance. |
| # | Evidence | Suggested fix | Trade-offs |
|---|---|---|---|
| 1 | Both arms differ only in the warn-log message already present; the print string + full CheckpointPolicyBlockedEvent{...} literal are identical. | Extract a private gitHookContext method covering the print + emit, called from both arms. | Minor — agents agreed it's low value since the blocks are 4 trivial fields. |
| 2 | No *_test.go references agentWriteHookLabel; the payload builder is tested but this mapping isn't. | Add a table test asserting post-todo / subagent-end / turn-end. | None. |
Observation (not a finding): a single blocked git commit fires three hook processes and produces three checkpoint_policy_blocked events distinguished by hook. This is the intended per-gate fan-out from our design (dedupe downstream) — flagged only so PostHog queries expect per-hook rows, not one-per-commit.
Summary: 0 Required, 2 Low Improvements. The branch is merge-ready on review grounds. The two Improvements are both about hardening the new code (dedup + a tiny test) and neither affects correctness.
Per the skill, there are no Autofix-eligible Required findings, so I'm stopping here rather than entering a fix cycle. Want me to apply either Improvement — in particular #2 (the agentWriteHookLabel table test), which I'd lean toward since it locks down an observable telemetry value cheaply?
Verification
| Command | Exit | Relevant output |
|---|---|---|
go build ./cmd/entire/cli/... | 0 | builds clean |
go test ./cmd/entire/cli/telemetry/ -count=1 | 0 | ok |
go test ./cmd/entire/cli/ -run 'TestAgentWriteHookLabel|Policy|HooksGit' -count=1 | 0 | ok |
go test ... -run TestAgentWriteHookLabel -v | 0 | PASS (3 subtests: post-todo, subagent-end, turn-end) |
mise run fmt && mise run lint | 0 | 0 issues |
Results
| # | Finding | File | Status | What changed | Why |
|---|---|---|---|---|---|
| 1 | Duplicated policy_unreadable arms | cmd/entire/cli/hooks_git_cmd.go | Fixed | Extracted skipUnreadableCheckpointPolicy(err) helper; both arms now call it | Single source of truth — the two identical blocks can't drift |
| 2 | agentWriteHookLabel untested | cmd/entire/cli/hook_registry_test.go | Fixed | Added TestAgentWriteHookLabel table test | Locks the observable hook telemetry label mapping |
| # | Files touched | Rollback notes |
|---|---|---|
| 1 | cmd/entire/cli/hooks_git_cmd.go | Revert the helper extraction — inline the warn+print+emit block back into both unreadable arms |
| 2 | cmd/entire/cli/hook_registry_test.go | Delete the TestAgentWriteHookLabel function |
Ledger: n/a (no project-local ./tmp/ artifact dir; kept inline above).
Finding 1 is a behavior-preserving refactor (covered by the existing git-hook tests, which still pass); finding 2 is a pure test addition. Both changes are uncommitted in the working tree — you decide what to commit. These are review fixes on top of the three existing telemetry commits, so they'd most naturally fold into a git commit --amend of the git-hook commit or land as a small follow-up commit; your call.
Committed. Review fixes are in fa9c791a92, leaving the pre-existing .claude/settings.json change untouched.
The review is complete:
- 6 agents, 0 Required findings. The telemetry change is sound on security, correctness, simplification, readability, and Go-cleanliness grounds.
- Both Low Improvements applied and verified (build + tests + lint all clean): deduped the git unreadable-policy skip path into a helper, and added the
agentWriteHookLabeltable test.
The branch now has 4 commits on top of the merge-base. Note it's still 32 commits behind origin/main — you'll want to rebase or merge before opening a PR so the eventual origin/main diff is clean. Let me know if you'd like anything else.