From 695a8665c2fef3ff706353a5b4ef19121f738a07 Mon Sep 17 00:00:00 2001 From: Michal Mironczuk Date: Tue, 28 Apr 2026 16:27:30 +0300 Subject: [PATCH 1/2] feat(agents): add ticket, review, and fix-comment workflow skills MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three new Claude Code / Cursor skills for the full Linear → code → PR loop: - /ticket — read Linear AC, plan, implement, verify, update status, post summary - /review — inventory all review threads (BLOCKER/CHANGE/QUESTION/NITPICK), fix/reply in priority order, push, reply per thread - /fix-comment — surgical single-thread fix + reply All three embed Karpathy's four principles (think before coding, simplicity first, surgical changes, goal-driven execution), pull Linear ticket context via MCP, use gh CLI for GitHub ops, and include escalation paths to existing skills (/analyze-tx for failing tx hashes, /add-network, /deprecate-network). Also adds a rule activation map and critical guards (whitelist branching, event locations, storage/selector impact, gas surfacing) to ticket.md, and rule-anchored response guidance (gas/security/architecture/events) to review.md. .claude/settings.json: add gh *, git checkout/commit/add/push/remote to allow list. Co-Authored-By: Claude Sonnet 4.6 --- .agents/commands/fix-comment.md | 120 ++++++++++++++ .agents/commands/review.md | 208 ++++++++++++++++++++++++ .agents/commands/ticket.md | 238 ++++++++++++++++++++++++++++ .claude/settings.json | 6 + .claude/skills/fix-comment/SKILL.md | 1 + .claude/skills/review/SKILL.md | 1 + .claude/skills/ticket/SKILL.md | 1 + .cursor/commands/fix-comment.md | 1 + .cursor/commands/review.md | 1 + .cursor/commands/ticket.md | 1 + 10 files changed, 578 insertions(+) create mode 100644 .agents/commands/fix-comment.md create mode 100644 .agents/commands/review.md create mode 100644 .agents/commands/ticket.md create mode 120000 .claude/skills/fix-comment/SKILL.md create mode 120000 .claude/skills/review/SKILL.md create mode 120000 .claude/skills/ticket/SKILL.md create mode 120000 .cursor/commands/fix-comment.md create mode 120000 .cursor/commands/review.md create mode 120000 .cursor/commands/ticket.md diff --git a/.agents/commands/fix-comment.md b/.agents/commands/fix-comment.md new file mode 100644 index 0000000000..1384169c90 --- /dev/null +++ b/.agents/commands/fix-comment.md @@ -0,0 +1,120 @@ +--- +name: fix-comment +description: Fix a single specific PR review comment/thread — understand, fix surgically, reply +usage: /fix-comment +--- + +# Fix Single PR Comment (`/fix-comment`) + +> **Usage**: `/fix-comment 312 r1234567` +> +> Focused single-thread version of `/review`. Reads exactly one comment, understands +> the full diff context around it, fixes or replies, pushes, marks resolved. +> Does not touch any other thread. + +--- + +## Inputs + +`$ARGUMENTS` — PR number and comment ID, space-separated. +- PR number: `312` +- Comment ID: `r1234567` (the `r` prefix is optional; numeric ID also accepted) + +Parse both values; abort with a clear message if either is missing. + +--- + +## Phase 0 — UNDERSTAND (before touching any file) + +> *"Don't assume. Don't hide confusion. Surface tradeoffs."* — Karpathy + +1. **Fetch the comment**: + ```bash + gh api repos/{owner}/{repo}/pulls/comments/$COMMENT_ID + ``` + Extract: body, file path, diff hunk (original_line, line), commit_id. + +2. **Resolve the Linear ticket** — extract ticket ID from branch name, PR title, or PR body + (e.g. `feat/eng-423-*` → `ENG-423`). If found, call `get_issue($ID)` and `list_comments($ID)` + to understand original intent and AC. Use when replying to QUESTION threads. + +3. **Read the diff context** — expand ±20 lines around the referenced line: + ```bash + gh api repos/{owner}/{repo}/pulls/$PR/files \ + --jq '.[] | select(.filename == "") | .patch' + ``` + Also read the current file state from disk. + +3. **State exactly what the reviewer is asking** — write it out in one sentence. + If the intent is ambiguous: + - Post a reply asking one focused clarifying question + - Stop — do not guess at the fix + +4. **Classify the request**: + - **BLOCKER** — correctness/security issue; must fix + - **CHANGE** — explicit edit; clear what to do + - **QUESTION** — asking why; answer in reply, code change only if "this is wrong" + - **NITPICK** — optional polish; fix only if unambiguous + +--- + +## Phase 1 — FIX + +> *"Touch only what you must. Clean up only your own mess."* — Karpathy + +**For BLOCKER / CHANGE**: +- Edit exactly the lines referenced by the comment — no more, no less +- No adjacent cleanup; no unasked renames; match existing style +- If fixing this line requires touching a dependency, note that explicitly before editing + +**For QUESTION**: +- Draft the reply text; only edit code if the reply would be "this is wrong" + +**For NITPICK**: +- Apply the polish if clear; skip if controversial (note in reply) + +Run the relevant verify checks immediately after the change: + +| Changed files | Command | +|---|---| +| `src/**/*.sol` | `bunx solhint ` + targeted `forge test` | +| `script/**/*.ts` | `bunx tsc-files --noEmit ` + `bunx eslint ` | +| Formatting | `bun format:fix` | + +Do not proceed to Phase 2 if any check fails. + +--- + +## Phase 2 — REPLY + +1. **Commit**: + ```bash + git commit -m "fix: address comment $COMMENT_ID on PR #$PR" + ``` + +2. **Push**: + ```bash + git push + ``` + +3. **Reply to the thread**: + ```bash + gh api repos/{owner}/{repo}/pulls/$PR/comments/$COMMENT_ID/replies \ + -f body="" + ``` + Reply format: + - **Fixed**: "Fixed in ``. ." + - **Question answered**: ". Let me know if you'd like more context." + - **Nitpick fixed**: "Done." + - **Skipped**: "Left as-is — . Happy to revisit if you feel strongly." + +--- + +## Abort Conditions + +- Comment ID not found → report the exact API error; do not guess another ID +- Intent is ambiguous → post a clarifying question; stop +- Comment references a specific on-chain tx hash → run `/analyze-tx ` first; + use the trace as source of truth before attempting any fix +- Fix requires Diamond storage layout change → stop, flag to user +- Tests fail after 1 fix attempt → report exact failure; stop diff --git a/.agents/commands/review.md b/.agents/commands/review.md new file mode 100644 index 0000000000..f626875145 --- /dev/null +++ b/.agents/commands/review.md @@ -0,0 +1,208 @@ +--- +name: review +description: Address all open PR review threads — categorize, triage, fix/reply, verify, push, mark resolved +usage: /review +--- + +# PR Review — Address Comments (`/review`) + +> **Usage**: `/review 312` +> +> Reads every open review thread on the PR, classifies each one, implements fixes +> in priority order, replies to each thread, and pushes — without touching anything +> beyond what reviewers explicitly asked for. + +--- + +## Inputs + +`$ARGUMENTS` — a GitHub PR number (e.g. `312`). + +Determine the repo from `git remote get-url origin`. Abort with a clear message if the +remote is not a GitHub URL. + +--- + +## Phase 0 — INVENTORY (before touching any file) + +> *"Don't assume. Don't hide confusion. Surface tradeoffs."* — Karpathy + +1. **Fetch PR metadata**: + ```bash + gh pr view $PR --json title,headRefName,baseRefName,body,author + ``` + +2. **Resolve the Linear ticket** — extract the ticket ID from the PR (in priority order): + - Branch name (e.g. `feat/eng-423-add-foo` → `ENG-423`) + - PR title (e.g. `ENG-423: add FooFacet`) + - PR body (look for `Closes ENG-423`, `Fixes ENG-423`, or a bare `ENG-423` reference) + + If a ticket ID is found: + - `get_issue($ID)` — title, description, acceptance criteria, status + - `list_comments($ID)` — prior discussion, design decisions, clarifications + + Use this context throughout: when a reviewer questions *why* something was done, + the ticket AC and discussion is the authoritative source for intent. + If no ticket ID is found, proceed without — note it in the Phase 1 triage output. + +3. **Fetch all review threads** (open + resolved, to understand context): + ```bash + gh api repos/{owner}/{repo}/pulls/$PR/comments --paginate + gh api repos/{owner}/{repo}/pulls/$PR/reviews --paginate + ``` + +3. **Build the thread inventory** — for every comment thread, record: + | Field | Value | + |---|---| + | Thread ID | `r` | + | File + line | `src/Facets/Foo.sol:42` | + | Reviewer | `@handle` | + | Body | (first 2 lines) | + | Status | open / resolved | + | Type | BLOCKER / CHANGE / QUESTION / NITPICK | + +4. **Type classification rules**: + - **BLOCKER** — breaks correctness, safety, or security; "this will X"; "bug:"; "wrong"; direct security concern; must be fixed before merge + - **CHANGE** — explicit edit request: naming, structure, docs, style; typically clear what to do + - **QUESTION** — asking why; asking for explanation; "why does this..."; "what is..." + - **NITPICK** — prefixed with "nit:", "minor:", "optional:", "feel free to ignore"; polish only + + When type is unclear, prefer the higher-severity classification. + +5. **Scan for tx-hash escalation triggers** — before classifying threads, check if any + thread references a specific transaction hash or says "this tx reverts / fails on-chain". + If found: run `/analyze-tx ` first. The trace output is the source of + truth for understanding the root cause; use it to inform your fix and your reply. + +6. **Print the inventory** to the user before proceeding to Phase 1. + +--- + +## Phase 1 — TRIAGE + +1. **Surface threads requiring a decision** — any BLOCKER where the right fix is + ambiguous. Ask **one focused question** per ambiguous BLOCKER. Wait for answer + before implementing that thread. + +2. **Fix order**: `BLOCKER` → `CHANGE` → `QUESTION replies` → `NITPICK` + +3. **Skip conditions** (document in final summary, do not silently omit): + - Thread already resolved (race condition) + - Reviewer explicitly said "optional, up to you" — note as skipped, explain why + - Thread conflicts with another — flag to user before picking a resolution + +--- + +## Phase 2 — IMPLEMENT + +> *"Touch only what you must. Clean up only your own mess."* — Karpathy + +**For BLOCKER / CHANGE threads**: +- Edit exactly the lines the reviewer references — no more, no less +- Do not refactor adjacent code that works correctly +- Do not rename things beyond the reviewer's explicit ask +- Match existing style; do not introduce new patterns unless asked + +**For QUESTION threads**: +- Draft a reply that explains the reasoning (reference file/line if helpful) +- Only make a code change if the explanation would be "this is wrong" — in which case reclassify as BLOCKER/CHANGE + +**For NITPICK threads**: +- Fix silently (no reply needed unless the fix is non-obvious) +- If the nitpick is controversial or conflicts with a rule, note in the PR summary comment and leave it + +**No new abstractions** — do not extract helpers, introduce shared utilities, or add +indirection unless at least two independent reviewer comments independently request it. + +**Rule-anchored responses** — when a reviewer raises a domain concern, ground your reply +in the relevant rule rather than improvising: + +| Reviewer raises | Use this anchor | What to say / check | +|---|---|---| +| Gas / efficiency concern | `106-gas` `[CONV:GAS]` | Prefer existing Solady/Solmate patterns; surface tradeoffs, don't micro-optimize silently | +| Security / access control | `105-security` | Check `Validatable`, `LibAsset`, `LibSwap`, `LibAllowList` for prior art; state governance impact | +| Architecture / Diamond concern | `002-architecture` `[CONV:ARCH-DIAMOND]` | Storage slots, selector layout, facet separation | +| Wrong event location | `002-architecture` `[CONV:EVENTS]` | `LiFiTransferStarted` only in `_startBridge`; `LiFiTransferCompleted` only in Executor | +| Whitelist file in wrong PR | `502-whitelist-branching` | PR must target `main`; split if targeting feature branch | +| Failing on-chain tx | `600-transaction-analysis` | Run `/analyze-tx ` first; reply with trace findings | + +--- + +## Phase 3 — VERIFY + +Run all checks relevant to files you changed. **State exact output; do not claim clean without running.** + +| Changed files | Command | +|---|---| +| `src/**/*.sol` | `forge test` (full suite or targeted) | +| `script/**/*.ts`, `tasks/**/*.ts` | `bun test:ts` + `bunx tsc-files --noEmit ` | +| Any `.sol` | `bunx solhint ` | +| Any `.ts`/`.js` | `bunx eslint ` | +| Formatting | `bun format:fix` | + +If any check fails, fix it before Phase 4. Do not push a red build. + +--- + +## Phase 4 — PUBLISH + +> *"Define success criteria. Loop until verified."* — Karpathy + +1. **Commit**: + ```bash + git commit -m "address PR #$PR review comments" + ``` + If fixes naturally split into logical units, use multiple commits with descriptive messages. + +2. **Push**: + ```bash + git push + ``` + +3. **Reply to each thread** — for every thread you acted on: + ```bash + # Reply to a comment thread + gh api repos/{owner}/{repo}/pulls/$PR/comments/$COMMENT_ID/replies \ + -f body="" + ``` + Reply format by type: + - **BLOCKER/CHANGE fixed**: "Fixed in . ." + - **QUESTION answered**: ". Happy to clarify further if needed." + - **NITPICK fixed**: "Done." (brief is fine for nitpicks) + - **NITPICK skipped**: "Left as-is — ." + +4. **Post a PR summary comment**: + ```bash + gh pr comment $PR --body "..." + ``` + ```markdown + ## Review comments addressed + + | Thread | File | Type | Action | + |---|---|---|---| + | r12345 | `src/Foo.sol:42` | CHANGE | Fixed — renamed to `fooBar` | + | r12346 | `src/Bar.sol:10` | QUESTION | Replied — explained reentrancy guard intent | + | r12347 | `src/Baz.sol:5` | NITPICK | Fixed silently | + + **Tests**: forge test — 42 passed, 0 failed + **Lint**: solhint — no issues + ``` + +--- + +## LI.FI Conventions (apply to any files you touch) + +- SPDX-License-Identifier: LGPL-3.0-only on all `.sol` files +- NatSpec on all public/external functions you modify +- `viem` only in TypeScript (no `ethers`) +- No Diamond storage layout changes without explicit ticket +- No governance bypass (Safe/timelock) + +--- + +## Abort Conditions + +- A BLOCKER's required fix is unclear and reviewer is unresponsive → post a comment + asking for clarification; do not guess; do not push +- A fix requires changing the Diamond storage layout → stop, flag to user +- Tests fail after 2 fix attempts → stop, report exact failure to user diff --git a/.agents/commands/ticket.md b/.agents/commands/ticket.md new file mode 100644 index 0000000000..986404567f --- /dev/null +++ b/.agents/commands/ticket.md @@ -0,0 +1,238 @@ +--- +name: ticket +description: End-to-end Linear ticket implementation — read, plan, implement, verify, update Linear status, post summary +usage: /ticket +--- + +# Ticket Implementation (`/ticket`) + +> **Usage**: `/ticket ENG-423` +> +> Runs the full loop: understand the ticket → plan → implement → verify → update Linear → post summary. + +--- + +## Inputs + +`$ARGUMENTS` — a Linear ticket ID (e.g. `ENG-423`, `INFRA-12`). + +Parse the workspace prefix and number; abort with a clear message if the format does not +match `[A-Z]+-\d+`. + +--- + +## Phase 0 — THINK (before touching any file) + +> *"Don't assume. Don't hide confusion. Surface tradeoffs."* — Karpathy + +1. **Fetch the ticket in full**: + - `get_issue($ID)` — title, description, status, priority, assignee, labels, parent + - `list_comments($ID)` — all prior discussion, reviewer notes, clarifications + - If linked/blocking issues exist: fetch each with `get_issue` + - If a PR is linked: note the branch name + +2. **Extract and echo back**: + - Acceptance criteria (numbered list; flag if none are explicit) + - Ambiguities or missing constraints + - Files/modules likely in scope based on the description + - Any "definition of done" hints in comments + +3. **State assumptions explicitly** — list every inference you made. + If any AC item is ambiguous, post a Linear comment asking one focused question + and **stop**. Do not implement until clarified. + +4. **Load applicable rules** — identify every rule that activates for the files you expect + to touch. Use this table: + + | Files you'll edit | Active rules | Key enforcements | + |---|---|---| + | `src/Facets/**/*.sol` | `100`, `101`, `102`, `105`, `106` | `[CONV:LICENSE]` `[CONV:NATSPEC]` `[CONV:EVENTS]` `[CONV:ARCH-DIAMOND]`, selector layout, event locations | + | `src/Interfaces/**/*.sol` | `100`, `103` | interface-only patterns, no logic | + | `src/Periphery/Receiver*.sol` | `100`, `104` | receiver-specific patterns | + | `src/**/*.sol` (other) | `100`, `101`, `105`, `106` | security, gas, NatSpec | + | `script/**/*.s.sol` | `100`, `107` | deploy script patterns | + | `script/**/*.ts`, `tasks/**/*.ts` | `200`, `105` | viem-only, no ethers, type safety | + | `script/deploy/safe/**/*.ts` | `200`, `201` | Safe/timelock decode conventions | + | `script/deploy/tron/**/*.ts` | `200`, `202` | TronWeb, address encoding | + | `test/**/*.t.sol` | `100`, `400`, `401` | test structure, naming, coverage expectations | + | `**/*.test.ts` | `402` | Bun test structure | + | `**/*.sh` | `300` | bash safety, exit codes | + | `config/whitelist.json` | `502` | **PR must target `main`** — see Critical Guards below | + | `.github/workflows/**` | `500` | immutable action SHAs, no tag refs | + + Always-active rules (apply regardless): `000`, `001`, `002`, `003`, `099`. + +--- + +## Phase 1 — PLAN + +> *"Minimum code that solves the problem. Nothing speculative."* — Karpathy + +1. **Define success criteria** — one bullet per AC item, phrased as a verifiable + test: "done when `forge test --match-test testFoo` passes" or + "done when the TS script exits 0 with expected output". + +2. **Identify files to change** — list paths. If you cannot identify them without + exploring, explore first; do not start editing until you know the full diff scope. + +3. **Choose the approach** — if two valid paths exist, name both with tradeoffs; + pick the smallest diff unless conventions or security justify the larger refactor. + No speculative abstractions; no helpers used by only one caller. + +4. **Output a compact plan**: + ``` + Branch: feat/ENG-423- + Files: src/Facets/FooFacet.sol, script/deploy/facets/DeployFoo.s.sol + Tests: forge test --match-contract FooFacetTest + Lint: bunx solhint src/Facets/FooFacet.sol + ``` + +--- + +## Phase 2 — IMPLEMENT + +> *"Touch only what you must. Clean up only your own mess."* — Karpathy + +1. **Branch** — always create a feature branch; never commit directly to `main`/`develop`: + ```bash + git checkout -b feat/- + # e.g. feat/eng-423-add-foo-facet + ``` + +2. **Edit only the files identified in Phase 1.** + - Do not improve adjacent code that works correctly. + - Do not rename identifiers beyond what the ticket asks. + - Match existing style, spacing, and comment density exactly. + - If you spot unrelated dead code, mention it in the summary — do not delete it. + +3. **Follow all active rules** — conventions from `.agents/rules/` apply to every file + you touch (`[CONV:LICENSE]`, `[CONV:NATSPEC]`, `[CONV:NAMING]`, etc.). + +4. **Critical Guards** — check these before committing: + + - **Whitelist files** (`config/whitelist.json`, `config/composerWhitelist.json`): the PR + **must target `main`**, not a feature branch. If your current branch targets a feature + branch, split the whitelist change into a separate PR targeting `main`. (rule `502`) + + - **Event emission** (`[CONV:EVENTS]`): `LiFiTransferStarted` only inside `_startBridge` + after all validations/calls; `LiFiTransferCompleted` only in `Executor.sol`; + `LiFiTransferRecovered` only in `Receiver*.sol`. Never emit these elsewhere. + + - **Storage / selector layout**: any change that could shift storage slots or selectors + must be called out explicitly in the Linear comment before merging. Do not silently + add storage variables to existing facets. (rule `002`) + + - **Gas** (`106`): call out any non-obvious gas tradeoffs in the summary so the user + can decide. Do not apply micro-optimizations silently. + + - **Security** (`105`): use `Validatable`, `LibAsset`, `LibSwap`, `LibAllowList` for + validation — no ad-hoc checks. Any admin-touching change must state governance impact. + +5. **Commit** when all files for a logical unit are done: + ``` + : + + # e.g. + ENG-423: add FooFacet with bar() swap path + ``` + One commit per logical unit; do not mix unrelated changes. + +--- + +## Phase 3 — VERIFY + +> *"Define success criteria. Loop until verified."* — Karpathy + +Run all checks relevant to the files you touched. **Do not claim done without running them.** + +| Changed files | Command | +|---|---| +| `src/**/*.sol` | `forge test` (full suite or `--match-contract `) | +| `script/**/*.s.sol` | `forge build` + dry-run if possible | +| `script/**/*.ts`, `tasks/**/*.ts` | `bun test:ts` + `bunx tsc-files --noEmit ` | +| Any `.sol` | `bunx solhint ` — fix all reported issues | +| Any `.ts`/`.js` | `bunx eslint ` — fix all reported issues | +| Formatting | `bun format:fix` | + +State the exact output (pass/fail/count) for each command run. +If a check cannot be run (e.g. no test suite for a new contract yet), say so explicitly +and create a follow-up note in the Linear comment. + +--- + +## Skill Escalation + +Some ticket types are better served by a dedicated skill. Escalate (invoke the skill) before +or instead of implementing manually: + +| Situation | Escalate to | +|---|---| +| `forge test` fails with a specific on-chain revert (has a tx hash) | `/analyze-tx ` — trace-first root cause analysis before fixing | +| Tests produce a suspicious revert you can't explain from code alone | `/analyze-tx` with a forked tx | +| Ticket asks to add a new chain/network | `/add-network` — handles networks.json, foundry.toml, permit2Proxy, gaszip, bridge configs | +| Ticket asks to remove/deprecate a network | `/deprecate-network` — removes from all config files safely | +| A previous audit finding is relevant to this ticket | Check `audit/auditLog.json`; use `/review-bounty-report` if a new bounty report is involved | + +After the escalated skill completes, return to this workflow (Phase 3 verify → Phase 4 publish). + +--- + +## Phase 4 — PUBLISH + +Only proceed here after Phase 3 is clean. + +1. **If open questions remain** — post a Linear comment with the questions, + set ticket to `In Progress` (or leave as-is), and stop. Wait for clarification. + +2. **Update ticket status**: + ``` + save_issue(id: $ID, stateId: ) + ``` + To find the right state ID, call `get_issue_status` or `list_issue_statuses` for the team. + +3. **Post summary comment** via `save_comment($ID, body: ...)`: + ```markdown + ## Implementation summary + + **Branch**: `feat/eng-423-add-foo-facet` + **Commit**: `ENG-423: add FooFacet with bar() swap path` + + ### What was done + - + + ### Tests + - `forge test`: 42 passed, 0 failed + - `bunx solhint src/Facets/FooFacet.sol`: no issues + + ### Open questions / follow-ups + - + ``` + +4. **Do not push** the branch unless the user explicitly asks to open a PR or push. + Leave branch local until requested. + +--- + +## LI.FI Conventions Checklist + +Before posting the summary, verify: + +- [ ] SPDX-License-Identifier: LGPL-3.0-only on every new `.sol` file +- [ ] NatSpec (`@notice`, `@param`, `@return`) on every public/external function +- [ ] Events emitted in correct locations (see `[CONV:EVENTS]` in rules) +- [ ] No interface or storage layout changes unless the ticket explicitly requires them +- [ ] Diamond selector conflicts checked if a new facet was added +- [ ] Safe + timelock governance flows not bypassed +- [ ] TypeScript uses `viem` (not `ethers`) +- [ ] Addresses normalized for multi-chain scripts (Tron detection if applicable) + +--- + +## Abort Conditions + +Stop and notify the user (do not implement) if: + +- AC is absent or contradictory and clarification returns nothing useful +- The ticket requires changes to the Diamond storage layout without a migration plan +- The ticket would bypass governance (Safe/timelock) — flag explicitly +- Tests fail and you cannot determine the root cause within 2 fix attempts diff --git a/.claude/settings.json b/.claude/settings.json index c095543530..477ef8d1ed 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -13,10 +13,16 @@ "Bash(git branch *)", "Bash(git show *)", "Bash(git stash list)", + "Bash(git checkout *)", + "Bash(git commit *)", + "Bash(git add *)", + "Bash(git push *)", + "Bash(git remote *)", "Bash(bash -n *)", "Bash(jq *)", "Bash(wc *)", "Bash(ls *)", + "Bash(gh *)", "Read", "Write", "Edit", diff --git a/.claude/skills/fix-comment/SKILL.md b/.claude/skills/fix-comment/SKILL.md new file mode 120000 index 0000000000..ba737caea5 --- /dev/null +++ b/.claude/skills/fix-comment/SKILL.md @@ -0,0 +1 @@ +../../../.agents/commands/fix-comment.md \ No newline at end of file diff --git a/.claude/skills/review/SKILL.md b/.claude/skills/review/SKILL.md new file mode 120000 index 0000000000..3f321d2465 --- /dev/null +++ b/.claude/skills/review/SKILL.md @@ -0,0 +1 @@ +../../../.agents/commands/review.md \ No newline at end of file diff --git a/.claude/skills/ticket/SKILL.md b/.claude/skills/ticket/SKILL.md new file mode 120000 index 0000000000..f77a798f78 --- /dev/null +++ b/.claude/skills/ticket/SKILL.md @@ -0,0 +1 @@ +../../../.agents/commands/ticket.md \ No newline at end of file diff --git a/.cursor/commands/fix-comment.md b/.cursor/commands/fix-comment.md new file mode 120000 index 0000000000..2ee7e6af81 --- /dev/null +++ b/.cursor/commands/fix-comment.md @@ -0,0 +1 @@ +../../.agents/commands/fix-comment.md \ No newline at end of file diff --git a/.cursor/commands/review.md b/.cursor/commands/review.md new file mode 120000 index 0000000000..390dda511c --- /dev/null +++ b/.cursor/commands/review.md @@ -0,0 +1 @@ +../../.agents/commands/review.md \ No newline at end of file diff --git a/.cursor/commands/ticket.md b/.cursor/commands/ticket.md new file mode 120000 index 0000000000..6019190ad0 --- /dev/null +++ b/.cursor/commands/ticket.md @@ -0,0 +1 @@ +../../.agents/commands/ticket.md \ No newline at end of file From 45e531b2e074b0eca8644b8a48687627a448be0f Mon Sep 17 00:00:00 2001 From: Michal Mironczuk Date: Tue, 28 Apr 2026 17:18:20 +0300 Subject: [PATCH 2/2] fix(agents): enhance review and fix-comment workflows with explicit confirmation steps Updated the review and fix-comment commands to require explicit user confirmation before proceeding. Added a new step to present the analysis and inventory to the user, ensuring clarity and agreement before moving to the next phase. This change aims to improve communication and accuracy in the workflow process. --- .agents/commands/fix-comment.md | 4 ++++ .agents/commands/review.md | 9 ++++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/.agents/commands/fix-comment.md b/.agents/commands/fix-comment.md index 1384169c90..3ecd50b618 100644 --- a/.agents/commands/fix-comment.md +++ b/.agents/commands/fix-comment.md @@ -56,6 +56,10 @@ Parse both values; abort with a clear message if either is missing. - **QUESTION** — asking why; answer in reply, code change only if "this is wrong" - **NITPICK** — optional polish; fix only if unambiguous +5. **Present the analysis and stop**. Show: what the reviewer is asking, the classification, and your proposed fix (or reply text). Ask: + > "Does this look right? Say yes to proceed, or give me corrections." + Wait for explicit confirmation before Phase 1. + --- ## Phase 1 — FIX diff --git a/.agents/commands/review.md b/.agents/commands/review.md index f626875145..2fd687903b 100644 --- a/.agents/commands/review.md +++ b/.agents/commands/review.md @@ -74,7 +74,9 @@ remote is not a GitHub URL. If found: run `/analyze-tx ` first. The trace output is the source of truth for understanding the root cause; use it to inform your fix and your reply. -6. **Print the inventory** to the user before proceeding to Phase 1. +6. **Print the inventory** to the user and **stop**. Ask: + > "Does this inventory look correct? Any threads to add, remove, or reclassify before I start fixing?" + Wait for explicit confirmation before proceeding to Phase 1. --- @@ -206,3 +208,8 @@ If any check fails, fix it before Phase 4. Do not push a red build. asking for clarification; do not guess; do not push - A fix requires changing the Diamond storage layout → stop, flag to user - Tests fail after 2 fix attempts → stop, report exact failure to user + +## Hard Rules + +- **Never approve the PR** (`gh pr review --approve` is forbidden). Post a summary comment only. +- **Never push or reply without user confirmation** — the Phase 0 inventory gate must be passed first.