Conversation
paullinator
force-pushed
the
paul/agents
branch
from
2c755f2 to
383ca7c
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
paullinator
force-pushed
the
paul/agents
branch
from
383ca7c to
1e9688d
Compare
paullinator
force-pushed
the
paul/agents
branch
3 times, most recently
from
ef2d703 to
cd2c769
Compare
paullinator
force-pushed
the
paul/agents
branch
from
cd2c769 to
a38ffe8
Compare
paullinator
force-pushed
the
paul/agents
branch
from
a38ffe8 to
5a7d612
Compare
This comment has been minimized.
This comment has been minimized.
paullinator
force-pushed
the
paul/agents
branch
from
5a7d612 to
6a441e7
Compare
swansontec
left a comment
swansontec
left a comment
There was a problem hiding this comment.
Submitting to preserve what I have so far.
.cursor/agents/review-cleaners.md
Outdated
| ```typescript | ||
| // To preserve explicit null values from API | ||
| const asResponse = asObject({ | ||
| data: asOptional(asEither(asNull, asString), null) |
There was a problem hiding this comment.
This code example doesn't do what it claims - null and undefined get merged together. Instead I would write:
"If you need special handling for null, you can create a custom cleaner function. For instance:"
/**
* Like `asOptional`, but explicitly preserves `null`.
*/
function asNullable<T>(cleaner: Cleaner<T>): Cleaner<T | null | undefined> {
return raw => {
if (raw === undefined) return undefined
if (raw === null) return null
return cleaner(raw)
}
}Alternatively, you could suggest: asEither(asValue(null, undefined), asString) This also does do what you want.
|
|
||
| --- | ||
|
|
||
| ## No Local Path Dependencies |
There was a problem hiding this comment.
We allow and even encourage this when we have linked PR's, such as a GUI update that depends on the core. We do want "edge-core-js": "../edge-core-js" in such a case, to be fixed once the core is published.
There was a problem hiding this comment.
if local path dep shows, the pr should have a dependent pr from the dependent library and that needs publish first before this can be merged.
.cursor/agents/review-errors.md
Outdated
|
|
||
| ## Understand undefined vs null Semantics | ||
|
|
||
| When checking for changes, understand that `undefined` and `null` have different meanings: |
There was a problem hiding this comment.
This will probably confuse the agent. We should clarify that != null is preferred:
When checking for changes, understand that
undefinedandnullhave different meanings. Prefer== nullto include both, unless we need to differentiate them:
// Correct: Testing if the thing exists
if (thing != null) {}
// Incorrect: Being too specific in checking for existence
if (thing !== undefined) {}
// Correct: We are using `null` to represent deleted items
if (changes[row] !== undefined) {}
.cursor/agents/review-errors.md
Outdated
|
|
||
| ## Don't Add Redundant Error Handling | ||
|
|
||
| When a global error handler already catches and displays errors, don't add local `.catch(showError)` calls: |
There was a problem hiding this comment.
This is redundant (ironically) with the "## Remove Unnecessary Catch Blocks" section above.
| --- | ||
|
|
||
| ## Handle User Cancellation Gracefully | ||
|
|
There was a problem hiding this comment.
This block seems bogus - we almost always return undefined to indicate cancellation in modals.
There is a new ExitError in the ramp plugin system. In that case, you want to use the handleExitErrorsGracefully helper, not explicit checks like these.
However, what we should really do is schedule an Asana task for @samholmes to delete ExitError and instead return undefined, just like the rest of the codebase. The ExitError pattern is dangerous, precisely because it's easy to forget like this. Return values force the type system to include the case.
.cursor/agents/review-async.md
Outdated
|
|
||
| ## Background Services | ||
|
|
||
| Background services go in `components/services/` directory: |
There was a problem hiding this comment.
The agent may not understand this. Be more explicit, like:
Do not spawn long-runnig
asyncwork in the background Instead, create a component in thecomponents/services/directory to manage the work's lifetime
.cursor/agents/review-async.md
Outdated
| }) | ||
| ``` | ||
|
|
||
| This prevents race conditions from multiple simultaneous calls to the same async function. |
There was a problem hiding this comment.
Delete this section. It's generally not needed, as EdgeTouchableOpacity and friends prevent concurrent taps.
This PR comment this is based on comes from a rather niche part of the code, and there are serious problems with the provided runOnce helper function which make it not generally applicable.
.cursor/agents/review-async.md
Outdated
| async processItem(id: string) { | ||
| try { | ||
| await this.fetch(id) | ||
| } catch (error) { | ||
| // BUG: This timeout runs even after killEngine() is called | ||
| setTimeout(() => this.processItem(id), 5000) | ||
| } | ||
| } |
There was a problem hiding this comment.
This is a horrible example - it should be using makePeriodicTask to begin with.
If we are going to give the LLM an example here, it should be a one-shot timeout, not a periodic task.
| value: asNumber | ||
| }) | ||
| type MyData = ReturnType<typeof asMyData> | ||
| ``` |
There was a problem hiding this comment.
Also add this:
// Correct - external types
import type { Foo } from 'some-library'
const asFoo = asObject(...)| } | ||
| await fetch('/api/users', { | ||
| method: 'POST', | ||
| body: JSON.stringify(body) |
There was a problem hiding this comment.
This is a bad example, because we should be using uncleaners in this case.
| ## Use Local Helpers for Amount Conversions | ||
|
|
||
| Avoid async wallet API calls for conversions since they cross an expensive bridge. Use local synchronous helpers instead: | ||
|
|
||
| ```typescript | ||
| // Incorrect - async bridge call for simple conversion | ||
| const amount = await wallet.nativeToDenomination(nativeAmount, currencyCode) | ||
|
|
||
| // Correct - use local helper with biggystring | ||
| import { div } from 'biggystring' | ||
| import { getExchangeDenom } from '../selectors/DenominationSelectors' | ||
|
|
||
| const { multiplier } = getExchangeDenom(wallet.currencyConfig, tokenId) | ||
| const exchangeAmount = div(nativeAmount, multiplier, DECIMAL_PRECISION) | ||
| ``` |
There was a problem hiding this comment.
I don't think we need this section - we handled this by deprecating EdgeCurrencyWallet.nativeToDenomination in the core. Maybe the advice should be simplified to "avoid deprecated methods".
|
|
||
| Use established libraries instead of implementing standard algorithms: | ||
|
|
||
| ```typescript | ||
| // Incorrect - hand-rolled base64 encoding | ||
| const toBase64 = (data: Uint8Array): string => { | ||
| let result = '' | ||
| for (let i = 0; i < data.length; i += 3) { | ||
| // ... manual base64 implementation | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
I would change this title to "Use rfc4648 for all base64 and hex conversions". While Buffer.toString('hex') is not "hand-rolled", it is also wrong.
.cursor/agents/review-react.md
Outdated
| ## Wrap Navigation After Gestures | ||
|
|
||
| When triggering navigation (push, pop, replace) immediately following a complex gesture (like a slider completion or swipe), wrap the navigation call in `InteractionManager.runAfterInteractions` to avoid crashes caused by unmounting components while the gesture system is active: | ||
|
|
||
| ```typescript | ||
| import { InteractionManager } from 'react-native' | ||
|
|
||
| // Incorrect - can crash on physical devices | ||
| const handleSliderComplete = () => { | ||
| navigation.pop() | ||
| } | ||
|
|
||
| // Correct - waits for gesture to finish | ||
| const handleSliderComplete = () => { | ||
| InteractionManager.runAfterInteractions(() => { | ||
| navigation.pop() | ||
| }) | ||
| } | ||
| ``` | ||
|
|
||
| This is especially important for slider-based confirmations (like send flows) where the navigation happens at the exact moment the gesture completes. |
There was a problem hiding this comment.
This is bad advice for 99% of cases.
I think this is a bug in the slider implementation, and the slider itself should should be doing runAfterInteractions(() => props.onSlideComplete()). Users of the slider should not be bothered with this, so we should create a follow-on task to fix the slider, if anything.
| }) | ||
| ``` | ||
|
|
||
| Pass the `pending` state to the component to visually indicate the loading state and/or disable the button. |
There was a problem hiding this comment.
Add this clarification:
Components that accept async callbacks like
onPressshould handle this internally. When passing anonPressprop toEdgeButton, for instance, the button itself handles this concern.
|
|
||
| The API process handles incoming HTTP requests and can run in cluster mode. The engine process runs background tasks, scheduled jobs, and long-running services. | ||
|
|
||
| ### PM2 Configuration |
There was a problem hiding this comment.
This should be merged with the "## Use PM2 for Process Management" section above
This comment has been minimized.
This comment has been minimized.
|
|
||
| ## Context-Based Key Names | ||
|
|
||
| String key names should describe their semantic meaning, not their location in the UI: |
There was a problem hiding this comment.
"Reuse Existing Strings" and "Context-Based Key Names" rules can cause some confusion.
Example, should we reuse the get_started_button as a scene title?
|
|
||
| ### Step 2: Fetch Bug-Fix PRs | ||
|
|
||
| Use GitHub MCP to search for bug-fix PRs: |
There was a problem hiding this comment.
should we update review-pr to include "bug" and "fix" in PR titles?
|
|
||
| ### View JavaScript Logs | ||
|
|
||
| The terminal running `yarn start` shows all `console.log` output from React Native JavaScript code. |
.cursor/skills/debug-edge/SKILL.md
Outdated
| yarn start | ||
| ``` | ||
|
|
||
| The debug server watches for changes and rebuilds automatically. Each repo needs its own terminal running `yarn start`. |
There was a problem hiding this comment.
The rebuilds are often <1 sec, but sometimes it can take >10 seconds. Add a line about waiting for the rebuild to finish before starting another testing loop
There was a problem hiding this comment.
Review Summary
Focusing on structural and integration issues — other reviewers have covered content/conventions thoroughly.
Deterministic scripts vs. semantic agent instructions
A recurring pattern across the commands and skills in this PR is relying on the agent to semantically interpret multi-step shell choreography (git workflows, API calls, build sequences) described in markdown. This works when the agent reasons correctly, but each step is a point of semantic failure. Companion shell scripts are testable, version-controlled, and produce consistent results regardless of which model or agent executes them. The agent's role should be deciding what to do (code changes, review logic), while deterministic operations (git fixup workflows, GitHub API calls, build commands) are better delegated to scripts the agent invokes.
For example, the fix-pr/SKILL.md fixup workflow involves 9 sequential git steps described in prose. A companion script like fixup-commit.sh <target-hash> could handle the mechanical git operations, reducing the agent's job to "call this script with the right hash." This is the pattern behind commit-autofix.sh — the agent doesn't manually run eslint, stage files, and commit; the script handles it atomically.
Finally, abstracting operations into shell scripts instead of letting the agent make sequential tool calls both preserves context and saves time. Each tool call the agent makes costs two context entries — the invocation and the output — and each carries per-call latency. A 9-step git workflow described in prose becomes 9 shell invocations, 9 outputs to parse, and 9 opportunities for the agent to misinterpret output or hit an unexpected state. When a step fails, the agent enters a reasoning loop: it reads the error, hypothesizes a fix, retries, reads the new output — each iteration burning context tokens and wall-clock time on what is fundamentally a deterministic problem.
A companion script collapses this into a single tool call with a single output. The script controls failure paths internally (exit codes, error messages the agent can act on), formats its output to give the agent exactly the information it needs (not raw stderr to parse), and handles retries or cleanup without round-tripping through the model. The agent's job reduces from "execute and interpret a multi-step procedure" to "call this script with the right arguments and react to its result."
Distribution model
The commands and skills work today in a multi-repo workspace where edge-conventions is open alongside target repos. However, Cursor has documented issues with rules discovery, glob patterns, and rule activation ordering in multi-root workspaces. An install script that syncs .cursor/ contents to ~/.cursor/ would provide a more reliable alternative — making commands, agents, and skills available globally regardless of workspace configuration, while keeping the source of truth version-controlled in this repo. This is analogous to Vercel's finding that passive context (always available in the prompt) achieves 100% agent compliance vs. 56-79% for active retrieval approaches.
MCP tool dependencies
Several skills rely on specific GitHub MCP being installed and available. This creates a hidden prerequisite — if a developer hasn't run install-mcp, or is relying on a different GitHub MCP (i.e. GitKraken MCP) the skills silently fail or the agent hallucinates tool calls. MCP tool invocation also relies on semantic reasoning to select the right tool from a catalog, adding another failure point. For GitHub API operations specifically, deterministic shell scripts using curl + $GITHUB_TOKEN are more portable, don't leave any ambiguity to semantics, and don't require MCP installation.
See inline comments for specific issues.
See: https://github.com/EdgeApp/edge-conventions/tree/jon/agents/.cursor/commands
https://github.com/EdgeApp/edge-conventions/blob/jon/agents/docs/agent-research/rule-compliance.md
| @@ -0,0 +1,92 @@ | |||
| # Agent Setup for Cursor | |||
There was a problem hiding this comment.
Distribution model is undefined. This PR adds commands, skills, and agents to edge-conventions/.cursor/, but doesn't document how these files reach developer machines. The current approach works in a multi-repo workspace where edge-conventions is open alongside target repos, but Cursor has documented bugs with rules discovery and glob patterns in multi-root workspaces (forum report).
Consider adding an install script (e.g., install.sh) that syncs .cursor/agents/, .cursor/commands/, and .cursor/skills/ to ~/.cursor/. This would eliminate multi-folder workspace bugs, make everything available globally, and resolve the @ path issues noted in other comments. The source of truth stays version-controlled here; the script just distributes it.
There was a problem hiding this comment.
There was a problem hiding this comment.
ugh. I would push back on this. Having everything in the workspace just automatically works without any need to copy files and keep things in sync.
.cursor/commands/debugedge.md
Outdated
| description: Debug the Edge app on iOS simulator | ||
| --- | ||
|
|
||
| Read and follow the instructions in @.cursor/skills/debug-edge/SKILL.md |
There was a problem hiding this comment.
@ path resolution depends on workspace root. @.cursor/skills/debug-edge/SKILL.md resolves relative to the workspace root. If the user is working in edge-react-gui (the common case), this resolves to edge-react-gui/.cursor/skills/debug-edge/SKILL.md, which doesn't exist — the file lives in edge-conventions/.cursor/skills/.
This affects all command files using @.cursor/skills/...: fixcode.md, fixpr.md, revcode.md, and codeit.md. If distribution moves to an install script syncing to ~/.cursor/, these would need absolute paths (e.g., @~/.cursor/skills/debug-edge/SKILL.md) or inlined content.
There was a problem hiding this comment.
Cursor will automatically look at the .cursor directory inside all projects in the workspace.
.cursor/skills/review-code/SKILL.md
Outdated
| Use the GitHub MCP server to get PR metadata including the head repository owner: | ||
|
|
||
| ``` | ||
| CallMcpTool: user-github / pull_request_read |
There was a problem hiding this comment.
CallMcpTool is not valid Cursor tool call syntax. This pseudo-syntax appears throughout skills and scrape commands (review-code/SKILL.md, fix-pr/SKILL.md, scrapebugs.md, scraperev.md). Cursor agents invoke MCP tools as regular function calls with names like user-github-list_pull_requests, user-github-get_pull_request, etc. The CallMcpTool: user-github / pull_request_read format won't be recognized.
The agent will either ignore these instructions or hallucinate tool calls. More fundamentally, MCP tool invocation still relies on semantic reasoning to pull up the right tool from the catalog, and each developer needs the same MCP servers installed. A deterministic shell script using curl + $GITHUB_TOKEN for GitHub API operations would be more portable — no MCP dependency, no semantic tool selection, works identically for every developer. See how pr-address.sh handles this: the agent decides what to do, the script handles the API call mechanics.
There was a problem hiding this comment.
I intend to have a major migration to use scripts in a future PR, but I don't want to make such a big change right now.
| ### Phase 2: Implementation | ||
|
|
||
| 1. Parse and execute the implementation steps from the planning document | ||
| 2. Do not commit changes unless the planning document explicitly specifies commits |
There was a problem hiding this comment.
Phase 2 "Do not commit" contradicts Phase 5 review loop. Phase 2 says "Do not commit changes unless the planning document explicitly specifies commits." But Phase 5 launches review-code/SKILL.md, which uses git diff <base>...HEAD to get changes. If nothing is committed, the diff is empty and the review has nothing to analyze.
Either Phase 2 should commit after implementation, or Phase 5 should diff unstaged changes (git diff without range) instead of committed history.
There was a problem hiding this comment.
Highly recommend to give "Plan Mode" natively available in Cursor a try. The DX is really smooth and functionally does what you are trying to do, possibly even better. I'm sure Cursor has a pretty thorough built-in system prompt they are using for that mode.
We can also customize "Plan mode" with our own workflow preferences, too.
There was a problem hiding this comment.
Also, I think this is out of scope for this PR. There is a lot to be improved based on the feedback in this PR and I think we should limit scope to hardening just the PR reviewer workflows.
|
|
||
| ## Implementing Fixes | ||
|
|
||
| Once the user approves the plan, process each fix item: |
There was a problem hiding this comment.
Fragile git workflow — standard fixup+autosquash is safer. The checkout-edit-fixup-cherry-pick approach (Steps 1-9) detaches HEAD, creates a fixup, then cherry-picks remaining commits. Any conflict during cherry-pick risks losing work and requires manual resolution for every fix item.
Standard approach is simpler and safer:
# Make the fix on the current branch
git commit --fixup <target-hash>
# After all fixes:
git rebase -i --autosquash <base>Additionally, this multi-step git choreography is a prime candidate for a companion shell script. The 9-step sequence described in prose is fragile when interpreted semantically by an agent — each step is a point of failure. A script like fixup-commit.sh <target-hash> would handle the mechanical git operations atomically, reducing the agent's job to "call this script with the right hash and the right files."
There was a problem hiding this comment.
| **Important**: Keep this terminal visible. React Native JavaScript logs appear here, making it essential for debugging JS-side issues. | ||
|
|
||
| ## Step 6: Build and Launch App | ||
|
|
There was a problem hiding this comment.
Missing timeout guidance for long-running operations. Step 6 (Build and Launch) can take 10-15 minutes, and Step 3 (yarn prepare.ios) can take several minutes. Without block_until_ms guidance, the agent will use the default 30s shell timeout, causing these commands to be backgrounded immediately and requiring unnecessary polling loops.
Add explicit timing guidance for slow steps, e.g.:
Run with
block_until_ms: 900000(15 min) — build can take 10-15 minutes on first run.
This also applies to codeit.md Phase 3 (build verification) and fix-code/SKILL.md (yarn precommit).
.cursor/commands/revcode.md
Outdated
|
|
||
| ## Instructions | ||
|
|
||
| Follow the `@.cursor/skills/rev-code/SKILL.md` skill to execute this command. |
There was a problem hiding this comment.
This link format (@.cursor/skills/rev-code/SKILL.md) doesn't always work correctly, causing misunderstanding and wasting tokens.
Depending on the setup, the real path might be <workspace>/edge-conventions/.cursor... for a workspace or even ~/.cursor/... if these are rules are installed globally (so every repo "just works" out of the box). The agent has to look around to figure out what the path means.
On the other hand, "use a named skill" is a tool available in the context. So, if you simply write Follow the "rev-code" skill..., the agent can run that tool directly and get to the actual review sooner.
There was a problem hiding this comment.
Yeah I had a similar comment regarding skills and file structure. Skills are non-deterministic in hit rate, best to just reference actual files.
There was a problem hiding this comment.
Yeah I had a similar comment regarding skills and file structure. Skills are non-deterministic in hit rate, best to just reference actual files.
.cursor/skills/review-code/SKILL.md
Outdated
|
|
||
| If a branch name is provided (not a PR): | ||
|
|
||
| 1. Identify the repository from the user's prompt or use the current working directory |
There was a problem hiding this comment.
I've noticed that the agent has a hard time with this step. Since we use slashes in our branch names ("william/fix-thing"), and since the instructions mention GitHub first, the agent treats branch names as a "repo/branch" GitHub URL, spends tokens to realize its mistake, and then falls back to local. Swapping the order points the agent in the right direction to start.
- 1. **GitHub PR URL**: e.g., `https://github.com/EdgeApp/edge-react-gui/pull/123`
- 2. **GitHub PR number**: e.g., `#123` or `PR 123` (requires repository context)
- 3. **Local branch name**: e.g., `feature/my-branch` or `paul/syncGitCouch`
- 4. **Current branch**: User says "review current branch" or similar
+ 1. **Current branch**: User says "review current branch" or similar
+ 2. **Local branch name**: e.g., `feature/my-branch` or `paul/syncGitCouch`
+ 3. **GitHub PR URL**: e.g., `https://github.com/EdgeApp/edge-react-gui/pull/123`
+ 4. **GitHub PR number**: e.g., `#123` or `PR 123` (requires repository context)And then:
- 1. Identify the repository from the user's prompt or use the current working directory
+ 1. Identify the repository from the current working directory or the user's promptThis seems to reduce the up-front misunderstanding, and starts the actual review more quickly.
There was a problem hiding this comment.
Review Summary
Focusing on structural and integration issues — other reviewers have covered content/conventions thoroughly.
Multi-agent review orchestration
The .cursor/agents/ directory contains 11 specialized review subagents launched in parallel by review-code/SKILL.md. This architecture has a fundamental cost/benefit problem.
Each subagent starts fresh with no inherited context — the orchestrator passes metadata (repo name, branch, file list), but every subagent must independently re-read the diff and changed files. For the same PR, that's 11x file reads of the same content. Combined with per-invocation model costs and latency, the overhead of fan-out exceeds the benefit of parallelism.
The Vercel AGENTS.md research found that compressed always-in-context documentation achieves 100% agent compliance, while agent-decides-to-invoke patterns (which the subagent dispatch model requires) achieve only 56%. The valuable content here is the review rules themselves — the project-specific conventions with concrete examples in each agent file. That content would be more effective consolidated into review standards file(s) loaded as context during a single-agent review pass, following the passive-pointer pattern that already works for typescript-standards.mdc.
Additionally, review-repo.md maps repository names to per-repo subagents (e.g., review-edge-react-gui) that don't exist in this PR. review-code/SKILL.md says to "compile findings from all subagents" and "check existing PR reviews to avoid duplicating feedback," but provides no mechanism for either — these are aspirational instructions without implementation.
Deterministic scripts vs. semantic agent instructions
A recurring pattern across the commands and skills in this PR is relying on the agent to semantically interpret multi-step shell choreography (git workflows, API calls, build sequences) described in markdown. This works when the agent reasons correctly, but each step is a point of semantic failure. Companion shell scripts are testable, version-controlled, and produce consistent results regardless of which model or agent executes them. The agent's role should be deciding what to do (code changes, review logic), while deterministic operations (git fixup workflows, GitHub API calls, build commands) are better delegated to scripts the agent invokes.
For example, the fix-pr/SKILL.md fixup workflow involves 9 sequential git steps described in prose. A companion script like fixup-commit.sh <target-hash> could handle the mechanical git operations, reducing the agent's job to "call this script with the right hash." This is the pattern behind commit-autofix.sh — the agent doesn't manually run eslint, stage files, and commit; the script handles it atomically.
Finally, abstracting operations into shell scripts instead of letting the agent make sequential tool calls both preserves context and saves time. Each tool call the agent makes costs two context entries — the invocation and the output — and each carries per-call latency. A 9-step git workflow described in prose becomes 9 shell invocations, 9 outputs to parse, and 9 opportunities for the agent to misinterpret output or hit an unexpected state. When a step fails, the agent enters a reasoning loop: it reads the error, hypothesizes a fix, retries, reads the new output — each iteration burning context tokens and wall-clock time on what is fundamentally a deterministic problem.
A companion script collapses this into a single tool call with a single output. The script controls failure paths internally (exit codes, error messages the agent can act on), formats its output to give the agent exactly the information it needs (not raw stderr to parse), and handles retries or cleanup without round-tripping through the model. The agent's job reduces from "execute and interpret a multi-step procedure" to "call this script with the right arguments and react to its result."
Distribution model
The commands and skills work today in a multi-repo workspace where edge-conventions is open alongside target repos. However, Cursor has documented issues with rules discovery, glob patterns, and rule activation ordering in multi-root workspaces. An install script that syncs .cursor/ contents to ~/.cursor/ would provide a more reliable alternative — making commands, agents, and skills available globally regardless of workspace configuration, while keeping the source of truth version-controlled in this repo. This is analogous to Vercel's finding that passive context (always available in the prompt) achieves 100% agent compliance vs. 56-79% for active retrieval approaches.
MCP tool dependencies
Several skills rely on specific GitHub MCP being installed and available. This creates a hidden prerequisite — if a developer hasn't run install-mcp, or is relying on a different GitHub MCP (i.e. GitKraken MCP) the skills silently fail or the agent hallucinates tool calls. MCP tool invocation also relies on semantic reasoning to select the right tool from a catalog, adding another failure point. For GitHub API operations specifically, deterministic shell scripts using curl + $GITHUB_TOKEN are more portable, don't leave any ambiguity to semantics, and don't require MCP installation.
See inline comments for specific issues.
See: https://github.com/EdgeApp/edge-conventions/tree/jon/agents/.cursor/commands
https://github.com/EdgeApp/edge-conventions/blob/jon/agents/docs/agent-research/rule-compliance.md
| @@ -0,0 +1,92 @@ | |||
| # Agent Setup for Cursor | |||
There was a problem hiding this comment.
Distribution model is undefined. This PR adds commands, skills, and agents to edge-conventions/.cursor/, but doesn't document how these files reach developer machines. The current approach works in a multi-repo workspace where edge-conventions is open alongside target repos, but Cursor has documented bugs with rules discovery and glob patterns in multi-root workspaces (forum report).
Consider adding an install script (e.g., install.sh) that syncs .cursor/agents/, .cursor/commands/, and .cursor/skills/ to ~/.cursor/. This would eliminate multi-folder workspace bugs, make everything available globally, and resolve the @ path issues noted in other comments. The source of truth stays version-controlled here; the script just distributes it.
More info https://github.com/EdgeApp/edge-conventions/blob/jon/agents/docs/agent-research/rule-compliance.md
.cursor/commands/debugedge.md
Outdated
| description: Debug the Edge app on iOS simulator | ||
| --- | ||
|
|
||
| Read and follow the instructions in @.cursor/skills/debug-edge/SKILL.md |
There was a problem hiding this comment.
@ path resolution depends on workspace root. @.cursor/skills/debug-edge/SKILL.md resolves relative to the workspace root. If the user is working in edge-react-gui (the common case), this resolves to edge-react-gui/.cursor/skills/debug-edge/SKILL.md, which doesn't exist — the file lives in edge-conventions/.cursor/skills/.
This affects all command files using @.cursor/skills/...: fixcode.md, fixpr.md, revcode.md, and codeit.md. If distribution moves to an install script syncing to ~/.cursor/, these would need absolute paths (e.g., @~/.cursor/skills/debug-edge/SKILL.md) or inlined content.
.cursor/skills/review-code/SKILL.md
Outdated
| Use the GitHub MCP server to get PR metadata including the head repository owner: | ||
|
|
||
| ``` | ||
| CallMcpTool: user-github / pull_request_read |
There was a problem hiding this comment.
CallMcpTool is not valid Cursor tool call syntax. This pseudo-syntax appears throughout skills and scrape commands (review-code/SKILL.md, fix-pr/SKILL.md, scrapebugs.md, scraperev.md). Cursor agents invoke MCP tools as regular function calls with names like user-github-list_pull_requests, user-github-get_pull_request, etc. The CallMcpTool: user-github / pull_request_read format won't be recognized.
The agent will either ignore these instructions or hallucinate tool calls. More fundamentally, MCP tool invocation still relies on semantic reasoning to pull up the right tool from the catalog, and each developer needs the same MCP servers installed. A deterministic shell script using curl + $GITHUB_TOKEN for GitHub API operations would be more portable — no MCP dependency, no semantic tool selection, works identically for every developer. See how pr-address.sh handles this: the agent decides what to do, the script handles the API call mechanics.
| ### Phase 2: Implementation | ||
|
|
||
| 1. Parse and execute the implementation steps from the planning document | ||
| 2. Do not commit changes unless the planning document explicitly specifies commits |
There was a problem hiding this comment.
Phase 2 "Do not commit" contradicts Phase 5 review loop. Phase 2 says "Do not commit changes unless the planning document explicitly specifies commits." But Phase 5 launches review-code/SKILL.md, which uses git diff <base>...HEAD to get changes. If nothing is committed, the diff is empty and the review has nothing to analyze.
Either Phase 2 should commit after implementation, or Phase 5 should diff unstaged changes (git diff without range) instead of committed history.
Highly recommend to give "Plan Mode" natively available in Cursor a try. The DX is really smooth and functionally does what you are trying to do, possibly even better. I'm sure Cursor has a pretty thorough built-in system prompt they are using for that mode.
We can also customize "Plan mode" with our own workflow preferences, too.
Also, I think this is out of scope for this PR. There is a lot to be improved based on the feedback in this PR and I think we should limit scope to hardening just the PR reviewer workflows.
There was a problem hiding this comment.
I added an explicit comment to fix the review code skill In the case of uncommitted changes.
|
|
||
| ## Implementing Fixes | ||
|
|
||
| Once the user approves the plan, process each fix item: |
There was a problem hiding this comment.
Fragile git workflow — standard fixup+autosquash is safer. The checkout-edit-fixup-cherry-pick approach (Steps 1-9) detaches HEAD, creates a fixup, then cherry-picks remaining commits. Any conflict during cherry-pick risks losing work and requires manual resolution for every fix item.
Standard approach is simpler and safer:
# Make the fix on the current branch
git commit --fixup <target-hash>
# After all fixes:
git rebase -i --autosquash <base>Additionally, this multi-step git choreography is a prime candidate for a companion shell script. The 9-step sequence described in prose is fragile when interpreted semantically by an agent — each step is a point of failure. A script like fixup-commit.sh <target-hash> would handle the mechanical git operations atomically, reducing the agent's job to "call this script with the right hash and the right files."
See https://github.com/EdgeApp/edge-conventions/tree/jon/agents/.cursor/commands
There was a problem hiding this comment.
As mentioned above, a future PR will extract deterministic operations into a script.
| **Important**: Keep this terminal visible. React Native JavaScript logs appear here, making it essential for debugging JS-side issues. | ||
|
|
||
| ## Step 6: Build and Launch App | ||
|
|
There was a problem hiding this comment.
Missing timeout guidance for long-running operations. Step 6 (Build and Launch) can take 10-15 minutes, and Step 3 (yarn prepare.ios) can take several minutes. Without block_until_ms guidance, the agent will use the default 30s shell timeout, causing these commands to be backgrounded immediately and requiring unnecessary polling loops.
Add explicit timing guidance for slow steps, e.g.:
Run with
block_until_ms: 900000(15 min) — build can take 10-15 minutes on first run.
This also applies to codeit.md Phase 3 (build verification) and fix-code/SKILL.md (yarn precommit).
.cursor/skills/review-code/SKILL.md
Outdated
| git symbolic-ref refs/remotes/origin/HEAD | sed 's@^refs/remotes/origin/@@' | ||
| ``` | ||
| 5. Skip the GitHub MCP sections and proceed directly to **Review Process** | ||
|
|
There was a problem hiding this comment.
There should be a condition for the local branch of uncommitted changes. In such a case, no git operations are needed.
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
paullinator
force-pushed
the
paul/agents
branch
from
73a3d4b to
ad6523b
Compare
This comment has been minimized.
This comment has been minimized.
.cursor/commands/codeit.md
Outdated
|
|
||
| 1. Report success to the user | ||
| 2. Summarize the implementation and any fixes made | ||
| 3. Restore stashed changes only if a stash was created in Phase 1 (run `git stash pop`) |
There was a problem hiding this comment.
Stash pop fails over uncommitted implementation changes
Medium Severity
Phase 1 stashes the user's existing uncommitted work, Phase 2 creates new uncommitted changes (explicitly instructed not to commit), and Phase 6 runs git stash pop while those Phase 2 changes are still in the working directory. If stashed files overlap with newly modified files, git stash pop will fail with merge conflicts, leaving the user's original work trapped in the stash.
Additional Locations (1)
| 5. Make the necessary code changes (edit files, stage them) | ||
| 6. Test changes with `yarn precommit` and fix any failures | ||
| 7. Stage and create the fixup commit: `git add -A && git commit --fixup HEAD --no-verify` | ||
| 8. Cherry-pick remaining commits: `git cherry-pick "${FIXUPHASH}..${BRANCHHEAD}"` |
There was a problem hiding this comment.
Cherry-pick fails when fixing the most recent commit
High Severity
When the reviewer's comment targets the most recent commit on the branch, FIXUPHASH equals BRANCHHEAD. The cherry-pick command git cherry-pick "${FIXUPHASH}..${BRANCHHEAD}" then receives an empty range, causing git to error out with a "bad revision" or "empty commit set" message. This is arguably the most common fix-pr scenario (reviewers typically comment on the latest changes), and the workflow has no guard to skip cherry-pick when there are no subsequent commits to replay.
Additional Locations (1)
This comment has been minimized.
This comment has been minimized.
samholmes
left a comment
samholmes
left a comment
There was a problem hiding this comment.
Very positive on all of these changes overall — great foundation for automating Edge development workflows. My feedback is mostly organizational: clearer naming, documenting skills alongside commands, and linking from the README. The highest priority item is the developer deployment story — right now there's no way to install these commands globally. The suggested scripts/install-cursor.sh approach (symlink .cursor/ subdirectories into ~/.cursor/edge/) should be the first thing in the setup doc and would make adoption straightforward.
.cursor/commands/revcode.md
Outdated
|
|
||
| ## Instructions | ||
|
|
||
| Follow the `@.cursor/skills/review-code/SKILL.md` skill to execute this command. |
There was a problem hiding this comment.
This command (and fixcode.md, fixpr.md, debugedge.md) is just a redirect to the corresponding skill. Since skills are already surfaced to the agent automatically and can be invoked by other skills/commands directly, these wrapper commands add a layer of indirection without adding value. Consider removing these four commands and just letting the skills serve as both the entry point and the implementation.
| @@ -0,0 +1,92 @@ | |||
| # Agent Setup for Cursor | |||
There was a problem hiding this comment.
This document isn't linked from the README.md table of contents. Since the README serves as the index for this repo and this doc is meant to be human-readable, it should be listed there (e.g. * [Agent Setup](docs/agent-setup.md)).
| @@ -0,0 +1,82 @@ | |||
| # codeit | |||
There was a problem hiding this comment.
Rename: codeit → implement-plan
.cursor/commands/packdep.md
Outdated
| @@ -0,0 +1,20 @@ | |||
| # packdep | |||
There was a problem hiding this comment.
Rename: packdep → pack-dependency
| @@ -0,0 +1,358 @@ | |||
| # scrapebugs | |||
There was a problem hiding this comment.
Rename: scrapebugs → scrape-bugs
| @@ -0,0 +1,372 @@ | |||
| # scraperev | |||
There was a problem hiding this comment.
Rename: scraperev → scrape-reviews
| # Agent Setup for Cursor | ||
|
|
||
| This document outlines how to set up AI-assisted development automation in Cursor for Edge development workflows. | ||
|
|
||
| ## MCP Server Installation | ||
|
|
||
| Run the `/edge-conventions/install-mcp` command to install the recommended MCP servers: |
There was a problem hiding this comment.
Missing: environment setup instructions. This document should start with how to install these utilities globally so they work from any Edge repo, not just when edge-conventions is the active workspace.
Suggested approach: add a shell script at scripts/install-cursor.sh that symlinks the .cursor/ subdirectories into ~/.cursor/ under an edge/ namespace. Step 1 in this doc becomes "clone the repo and run the script":
git clone git@github.com:EdgeApp/edge-conventions.git
cd edge-conventions
./scripts/install-cursor.shThe script itself (scripts/install-cursor.sh):
#!/bin/bash
SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
REPO_CURSOR="$SCRIPT_DIR/../.cursor"
for dir in commands skills agents; do
target="$HOME/.cursor/$dir/edge"
if [ -e "$target" ]; then
read -p "$target already exists. Overwrite? [y/N] " answer
if [ "$answer" != "y" ]; then
echo "Skipping $dir"
continue
fi
rm "$target"
fi
mkdir -p "$HOME/.cursor/$dir"
ln -s "$REPO_CURSOR/$dir" "$target"
echo "Linked $target → $REPO_CURSOR/$dir"
doneThis way:
git pullin edge-conventions automatically updates the global commands (they're symlinks, not copies)- No collision with the user's own commands/skills/agents (everything is namespaced under
/edge) - If the user moves their clone, they just re-run
./scripts/install-cursor.sh - Zero agent tokens — it's a plain shell script
The MCP server installation section can stay after this, as step 2.
|
|
||
| --- | ||
|
|
||
| ## Cursor Commands |
There was a problem hiding this comment.
This section documents commands but there's no corresponding Cursor Skills section. Skills like review-code, fix-pr, fix-code, and debug-edge are the actual implementations behind several commands and can also be invoked directly by other skills/commands. They deserve their own section in this doc.
|
|
||
| --- | ||
|
|
||
| ## Best Practices |
There was a problem hiding this comment.
Missing: scraperev and scrapebugs are not documented here. These are the commands that mine merged PRs for review patterns and bug-fix root causes, feeding new rules back into the review agents. They're a key part of the system's feedback loop.
Given their complexity (long-running, GitHub API rate limits, resume support, multiple parameters), they warrant a dedicated doc rather than cramming the details into this overview. Suggested approach:
-
Add a brief section here (before Best Practices) summarizing what they do:
GitHub PR Scraping
Two commands analyze merged PRs to discover new review patterns:
- scrape-reviews — Extracts accepted reviewer feedback and adds rules to review agents
- scrape-bugs — Analyzes bug-fix PRs for root causes and proposes preventive rules
See GitHub PR Scraping for usage, parameters, and examples.
-
Create
docs/github-pr-scraping.mdwith the detailed usage (parameters, examples, rate limit handling, resume behavior).
| number=$(echo "$pr_json" | jq -r '.number') | ||
| head_sha=$(echo "$pr_json" | jq -r '.headRefOid') | ||
| owner=$(echo "$pr_json" | jq -r '.headRepositoryOwner.login') | ||
| repo=$(echo "$pr_json" | jq -r '.headRepository.name') |
There was a problem hiding this comment.
Script extracts fork owner instead of base repository
Medium Severity
cmd_start extracts owner and repo from headRepositoryOwner and headRepository, which refer to the source (fork) repository. Pull requests live on the base (target) repository, so all subsequent API calls — the GraphQL pending-review query, the REST review-creation call, and the owner/repo values propagated to summary, submit, and discard — target the wrong repository when the PR comes from a fork. The url field (which always points to the base repo) could be parsed instead.
Additional Locations (1)
This comment has been minimized.
This comment has been minimized.
Remove unnecessary commands
Add the gh-review skill for orchestrating interactive GitHub PR review sessions via the gh CLI. Document all five skills (review-code, fix-pr, fix-code, debug-edge, gh-review) in a new Cursor Skills section in agent-setup.md.
The script was extracting owner and repo from headRepositoryOwner and headRepository, which refer to the fork (source) repository. For fork PRs, this caused all API calls to target the wrong repository since PRs live on the base (target) repository. Fixed by parsing owner and repo from the URL field, which always points to the base repository where the PR actually exists.
paullinator
force-pushed
the
paul/agents
branch
from
15971ba to
0e65f88
Compare
|
|
||
| **Tip**: For plugin repo debugging (edge-core-js, edge-currency-accountbased, edge-exchange-plugins, edge-currency-plugins), use `log.warn()` instead of `console.log` to see output in Metro logs. Enable the corresponding `DEBUG_*` flag in `env.json` and run `yarn start` in the plugin repo for hot-reload. | ||
|
|
||
| --- |
There was a problem hiding this comment.
Duplicated "Skills" sections with inconsistent coverage
Low Severity
The document has two overlapping sections: ## Skills (line 31) and ## Cursor Skills (line 82), both describing the same .cursor/skills/ directory. Four skills (review-code, fix-pr, fix-code, debug-edge) are described in both sections with slightly different wording. Meanwhile, package-deps only appears in the first section and gh-review only in the second. These two sections need to be merged into one with complete, non-duplicated coverage.
Additional Locations (1)
This comment has been minimized.
This comment has been minimized.
1. Use gh instead of github mcp 2. Allow for non-interactive review 3. Remove reference to cursor workspaces
|
|
||
| **Workflow**: Start or resume a pending review → Add inline comments interactively (single-line or multi-line) via GraphQL API → Present summary of all comments → Submit only after explicit user confirmation as APPROVE, COMMENT, or REQUEST_CHANGES | ||
|
|
||
| **Key detail**: Uses GraphQL `addPullRequestReviewThread` for incremental comments because the REST API returns 422 when a pending review already exists. |
There was a problem hiding this comment.
Duplicate skill descriptions in setup documentation
Low Severity
The document has two nearly identical sections: "## Skills" (lines 31–80) and "## Cursor Skills" (lines 83–143). Both describe review-code, fix-pr, fix-code, and debug-edge with slightly varying detail levels. The second section adds gh-review but otherwise duplicates the first. This creates a maintenance burden where updates need to happen in two places.
Additional Locations (1)
| console.log(` Removing old tarball: ${file}`) | ||
| fs.unlinkSync(path.join(guiDir, file)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Tarball cleanup prefix match is overly broad
Low Severity
The old tarball cleanup uses file.startsWith(namePrefix + '-') to find previous tarballs. For a package like edge-core, the prefix edge-core- also matches tarballs for edge-core-js, causing unintended deletion. A more precise regex matching the version portion (e.g., a digit after the prefix) would prevent false matches against similarly-named packages.
This comment has been minimized.
This comment has been minimized.
Helps improve speed and reduce token usage
| const flags = {} | ||
| const positional = [] | ||
| for (let i = 0; i < rawArgs.length; i++) { | ||
| if (rawArgs[i] === '--base') flags.base = rawArgs[++i] |
There was a problem hiding this comment.
--base flag parsed but never applied to baseBranch
Medium Severity
The --base flag is parsed into flags.base and advertised in the usage message, but flags.base is never referenced anywhere in the file. The baseBranch variable is set from PR metadata or a git symbolic-ref command, completely ignoring the user-supplied override. Passing --base main silently does nothing.
Additional Locations (1)
| }) | ||
| .split('\n') | ||
| .filter(Boolean) | ||
| changedFiles = [...new Set([...uFiles, ...sFiles])] |
There was a problem hiding this comment.
Failed base-branch check silently skips all committed changes
High Severity
The hasCommits check on line 205 uses allowFailure: true, so when baseBranch doesn't exist as a git ref (e.g., fallback 'master' on a repo using main), git log fails and returns ''. The condition isLocalBranch && !hasCommits then evaluates to true, silently falling into the "uncommitted-only changes" path. This means all committed work on the branch is completely ignored — the tool reviews only unstaged/staged changes and produces an empty or incomplete diff with no warning. The hasCommits check conflates "git command failed" with "no commits between base and HEAD."
| **Workflow**: Install deps → Clean iOS build artifacts → Prepare iOS → Clean Xcode build → Start Metro bundler → Build and launch app → Log into test account if needed | ||
|
|
||
| **Tip**: For plugin repo debugging (edge-core-js, edge-currency-accountbased, edge-exchange-plugins, edge-currency-plugins), use `log.warn()` instead of `console.log` to see output in Metro logs. Enable the corresponding `DEBUG_*` flag in `env.json` and run `yarn start` in the plugin repo for hot-reload. | ||
|
|
There was a problem hiding this comment.
Duplicate "Skills" and "Cursor Skills" documentation sections
Low Severity
The document has a ## Skills section (lines 31–80) and a ## Cursor Skills section (lines 83–130) that both describe the same concepts. review-code, fix-pr, fix-code, and debug-edge are each documented twice with slightly different wording. This creates a maintenance burden and may confuse readers about which description is canonical. The unique content (gh-review) only appears in the second section.
Additional Locations (1)
|
Bugbot Autofix prepared fixes for 3 of the 3 bugs found in the latest run.
Or push these changes by commenting: Preview (9411e8cdcf)diff --git a/.cursor/skills/review-code/scripts/review-prep.js b/.cursor/skills/review-code/scripts/review-prep.js
--- a/.cursor/skills/review-code/scripts/review-prep.js
+++ b/.cursor/skills/review-code/scripts/review-prep.js
@@ -193,6 +193,11 @@
) || 'master'
}
+// Apply --base override if provided
+if (flags.base) {
+ baseBranch = flags.base
+}
+
// ---------------------------------------------------------------------------
// Diff generation
// ---------------------------------------------------------------------------
@@ -202,6 +207,29 @@
let diff
let changedFiles
+// Verify base branch exists before attempting to diff against it
+let baseBranchExists = run(`git rev-parse --verify ${baseBranch}`, {
+ cwd: repoDir,
+ allowFailure: true
+})
+
+if (isLocalBranch && !baseBranchExists) {
+ log(`Warning: Base branch '${baseBranch}' not found. Trying 'origin/${baseBranch}'...`)
+ const originBase = `origin/${baseBranch}`
+ const originExists = run(`git rev-parse --verify ${originBase}`, {
+ cwd: repoDir,
+ allowFailure: true
+ })
+ if (originExists) {
+ baseBranch = originBase
+ baseBranchExists = originExists
+ } else {
+ log(`Error: Base branch '${baseBranch}' does not exist locally or on origin.`)
+ log('Please specify a valid base branch with --base <branch>')
+ process.exit(1)
+ }
+}
+
const hasCommits = run(`git log --oneline ${baseBranch}..HEAD`, {
cwd: repoDir,
allowFailure: true
diff --git a/docs/agent-setup.md b/docs/agent-setup.md
--- a/docs/agent-setup.md
+++ b/docs/agent-setup.md
@@ -30,7 +30,7 @@
## Skills
-Skills are defined in `.cursor/skills/` and are automatically invoked by Cursor when relevant. They can also be referenced from commands via `@.cursor/skills/<name>/SKILL.md`.
+Skills are defined in `.cursor/skills/` and provide reusable workflows that commands and other skills can invoke. They are automatically invoked by Cursor when relevant and can also be referenced from commands via `@.cursor/skills/<name>/SKILL.md`.
### debug-edge
@@ -38,10 +38,12 @@
**Prerequisites**: Xcode, working directory `edge-react-gui`, MCP servers `xcodebuild` and `mobile-mcp`
-**Workflow**: Install deps → Clean iOS → Prepare iOS → Start Metro → Build and launch → Login if needed
+**Workflow**: Install deps → Clean iOS build artifacts → Prepare iOS → Clean Xcode build → Start Metro bundler → Build and launch app → Log into test account if needed
-**Tip**: For plugin repo debugging (edge-core-js, edge-currency-accountbased, edge-exchange-plugins, edge-currency-plugins), use `log.warn()` instead of `console.log` to see output in Metro logs.
+**Tip**: For plugin repo debugging (edge-core-js, edge-currency-accountbased, edge-exchange-plugins, edge-currency-plugins), use `log.warn()` instead of `console.log` to see output in Metro logs. Enable the corresponding `DEBUG_*` flag in `env.json` and run `yarn start` in the plugin repo for hot-reload.
+---
+
### fix-code
Implement fixes from a plan document by editing code.
@@ -52,48 +54,8 @@
**Constraints**: Does not commit or push. Leaves changes unstaged for user review.
-### fix-pr
-
-Address reviewer comments on a GitHub pull request.
-
-**Usage**: Provide a GitHub PR URL or number.
-
-**Workflow**: Collect reviewer comments → Create fix plan → Pause for user iteration → Implement fixes with fixup commits → Push with `--force-with-lease`
-
-**Constraints**: Does not auto-squash. Leaves fixup commits visible for reviewer.
-
-### package-deps
-
-Package local dependency repos and link them to edge-react-gui for testing.
-
-**Usage**: Specify dependency repos to package.
-
-**Workflow**: `npm pack` → Rename with UTC timestamp → Copy to edge-react-gui → Update `package.json` reference
-
-### review-code
-
-Review code changes for quality and convention compliance.
-
-**Usage**: Provide a GitHub PR URL, PR number, local branch name, or "current branch".
-
-**Workflow**: Checkout code → Get diff → Launch review subagents (`review-react`, `review-errors`, `review-state`, etc.) → Compile findings → Save to `/tmp` → For PRs: submit inline comments via GitHub MCP
-
---
-## Cursor Skills
-
-Skills are defined in `.cursor/skills/` and provide reusable workflows that commands and other skills can invoke. They are also surfaced to the agent automatically and can be triggered directly.
-
-### review-code
-
-Review code changes for quality and convention compliance. Supports both GitHub pull requests and local branches.
-
-**Usage**: Provide a GitHub PR URL, PR number, local branch name, or "current branch".
-
-**Workflow**: Checkout code → Detect fork vs internal branch → Get diff → Launch review subagents in parallel (`review-react`, `review-errors`, `review-state`, `review-async`, `review-cleaners`, `review-code-quality`, `review-comments`, `review-strings`, `review-tests`, `review-pr`, `review-servers`, `review-repo`) → Compile findings into Critical Issues / Warnings / Suggestions → Save review to `/tmp` → For PRs: submit inline comments via GitHub
-
----
-
### fix-pr
Address reviewer comments on a GitHub pull request.
@@ -106,42 +68,38 @@
---
-### fix-code
+### gh-review
-Implement fixes from a plan document by editing code.
+Orchestrate an interactive GitHub PR review session using the `gh` CLI.
-**Usage**: Provide a plan document path (typically generated by `fix-pr` or `review-code`).
+**Prerequisites**: `gh` CLI installed and authenticated
-**Workflow**: Parse plan → Edit files → Run `yarn precommit` → Present summary
+**Usage**: Provide a PR reference (URL, number, or "current PR") and comment instructions.
-**Constraints**: Does not commit or push. Leaves changes unstaged for user review.
+**Workflow**: Start or resume a pending review → Add inline comments interactively (single-line or multi-line) via GraphQL API → Present summary of all comments → Submit only after explicit user confirmation as APPROVE, COMMENT, or REQUEST_CHANGES
+**Key detail**: Uses GraphQL `addPullRequestReviewThread` for incremental comments because the REST API returns 422 when a pending review already exists.
+
---
-### debug-edge
+### package-deps
-Compile, launch, and debug the Edge wallet app on an iOS simulator.
+Package local dependency repos and link them to edge-react-gui for testing.
-**Prerequisites**: Xcode, working directory `edge-react-gui`, MCP servers `xcodebuild` and `mobile-mcp`
+**Usage**: Specify dependency repos to package.
-**Workflow**: Install deps → Clean iOS build artifacts → Prepare iOS → Clean Xcode build → Start Metro bundler → Build and launch app → Log into test account if needed
+**Workflow**: `npm pack` → Rename with UTC timestamp → Copy to edge-react-gui → Update `package.json` reference
-**Tip**: For plugin repo debugging (edge-core-js, edge-currency-accountbased, edge-exchange-plugins, edge-currency-plugins), use `log.warn()` instead of `console.log` to see output in Metro logs. Enable the corresponding `DEBUG_*` flag in `env.json` and run `yarn start` in the plugin repo for hot-reload.
-
---
-### gh-review
+### review-code
-Orchestrate an interactive GitHub PR review session using the `gh` CLI.
+Review code changes for quality and convention compliance. Supports both GitHub pull requests and local branches.
-**Prerequisites**: `gh` CLI installed and authenticated
+**Usage**: Provide a GitHub PR URL, PR number, local branch name, or "current branch".
-**Usage**: Provide a PR reference (URL, number, or "current PR") and comment instructions.
+**Workflow**: Checkout code → Detect fork vs internal branch → Get diff → Launch review subagents in parallel (`review-react`, `review-errors`, `review-state`, `review-async`, `review-cleaners`, `review-code-quality`, `review-comments`, `review-strings`, `review-tests`, `review-pr`, `review-servers`, `review-repo`) → Compile findings into Critical Issues / Warnings / Suggestions → Save review to `/tmp` → For PRs: submit inline comments via GitHub
-**Workflow**: Start or resume a pending review → Add inline comments interactively (single-line or multi-line) via GraphQL API → Present summary of all comments → Submit only after explicit user confirmation as APPROVE, COMMENT, or REQUEST_CHANGES
-
-**Key detail**: Uses GraphQL `addPullRequestReviewThread` for incremental comments because the REST API returns 422 when a pending review already exists.
-
---
## Best Practices |
Introduce dedicated skills for branch cleanup commits and PR submission so execute-plan can finish with a consistent local commit step followed by structured PR creation. Made-with: Cursor
Document simulator video capture, artifact directory handling, and timestamped screenshot naming so automated debug runs preserve reproducible evidence in a shared temp workspace. Made-with: Cursor
Replace user-specific absolute paths with ~/git-based paths so guidance remains portable across developer environments. Made-with: Cursor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Parsed
--baseflag is never used- The script now prioritizes
flags.basewhen settingbaseBranchin both PR and local-branch flows.
- The script now prioritizes
- ✅ Fixed: Renamed files missed by pattern-based subagent routing
- Diff parsing now indexes each file section by both
a/andb/paths so renamed files map correctly during pattern-based routing.
- Diff parsing now indexes each file section by both
- ✅ Fixed: Duplicated
ensureGhTokenfunction across two scripts- I extracted
ensureGhTokeninto a sharedgh-token.jshelper and imported it from both scripts to remove duplication.
- I extracted
Or push these changes by commenting:
@cursor push 62a0e55f51
Preview (62a0e55f51)
diff --git a/.cursor/skills/review-code/scripts/gh-token.js b/.cursor/skills/review-code/scripts/gh-token.js
new file mode 100644
--- /dev/null
+++ b/.cursor/skills/review-code/scripts/gh-token.js
@@ -1,0 +1,21 @@
+'use strict'
+
+const { execSync } = require('child_process')
+
+function ensureGhToken() {
+ if (process.env.GH_TOKEN) return
+ try {
+ const b64 = execSync(
+ 'security find-generic-password -s "gh:github.com" -w 2>/dev/null',
+ { encoding: 'utf8' }
+ ).trim()
+ const match = b64.match(/^go-keyring-base64:(.+)$/)
+ if (match) {
+ process.env.GH_TOKEN = Buffer.from(match[1], 'base64').toString('utf8')
+ } else if (b64.startsWith('gho_') || b64.startsWith('ghp_')) {
+ process.env.GH_TOKEN = b64
+ }
+ } catch (_) {}
+}
+
+module.exports = { ensureGhToken }
diff --git a/.cursor/skills/review-code/scripts/review-prep.js b/.cursor/skills/review-code/scripts/review-prep.js
--- a/.cursor/skills/review-code/scripts/review-prep.js
+++ b/.cursor/skills/review-code/scripts/review-prep.js
@@ -4,27 +4,12 @@
const { execSync } = require('child_process')
const path = require('path')
const fs = require('fs')
+const { ensureGhToken } = require('./gh-token')
// ---------------------------------------------------------------------------
// Helpers
// ---------------------------------------------------------------------------
-function ensureGhToken() {
- if (process.env.GH_TOKEN) return
- try {
- const b64 = execSync(
- 'security find-generic-password -s "gh:github.com" -w 2>/dev/null',
- { encoding: 'utf8' }
- ).trim()
- const match = b64.match(/^go-keyring-base64:(.+)$/)
- if (match) {
- process.env.GH_TOKEN = Buffer.from(match[1], 'base64').toString('utf8')
- } else if (b64.startsWith('gho_') || b64.startsWith('ghp_')) {
- process.env.GH_TOKEN = b64
- }
- } catch (_) {}
-}
-
ensureGhToken()
function run(cmd, opts = {}) {
@@ -135,7 +120,7 @@
)
branchName = prMeta.headRefName
- baseBranch = prMeta.baseRefName
+ baseBranch = flags.base || prMeta.baseRefName
const headOwner = prMeta.headRepositoryOwner.login
prUrl = prUrl || prMeta.url
const isFork = headOwner !== owner
@@ -187,10 +172,12 @@
}
baseBranch =
+ flags.base ||
run(
"git symbolic-ref refs/remotes/origin/HEAD 2>/dev/null | sed 's@^refs/remotes/origin/@@'",
{ cwd: repoDir, allowFailure: true }
- ) || 'master'
+ ) ||
+ 'master'
}
// ---------------------------------------------------------------------------
@@ -257,8 +244,11 @@
function parseDiffByFile(raw) {
const result = {}
for (const part of raw.split(/^diff --git /m).filter(Boolean)) {
- const m = part.match(/a\/(.+?) b\//)
- if (m) result[m[1]] = part
+ const m = part.match(/^a\/(.+?) b\/(.+)$/m)
+ if (m) {
+ result[m[1]] = part
+ result[m[2]] = part
+ }
}
return result
}
diff --git a/.cursor/skills/review-code/scripts/review-submit.js b/.cursor/skills/review-code/scripts/review-submit.js
--- a/.cursor/skills/review-code/scripts/review-submit.js
+++ b/.cursor/skills/review-code/scripts/review-submit.js
@@ -3,23 +3,8 @@
const { execSync } = require('child_process')
const fs = require('fs')
+const { ensureGhToken } = require('./gh-token')
-function ensureGhToken() {
- if (process.env.GH_TOKEN) return
- try {
- const b64 = execSync(
- 'security find-generic-password -s "gh:github.com" -w 2>/dev/null',
- { encoding: 'utf8' }
- ).trim()
- const match = b64.match(/^go-keyring-base64:(.+)$/)
- if (match) {
- process.env.GH_TOKEN = Buffer.from(match[1], 'base64').toString('utf8')
- } else if (b64.startsWith('gho_') || b64.startsWith('ghp_')) {
- process.env.GH_TOKEN = b64
- }
- } catch (_) {}
-}
-
ensureGhToken()
function run(cmd) {| const flags = {} | ||
| const positional = [] | ||
| for (let i = 0; i < rawArgs.length; i++) { | ||
| if (rawArgs[i] === '--base') flags.base = rawArgs[++i] |
There was a problem hiding this comment.
Parsed --base flag is never used
Medium Severity
The --base CLI flag is parsed into flags.base but never referenced when setting baseBranch. The baseBranch variable is always determined from PR metadata (prMeta.baseRefName) or git symbolic-ref, completely ignoring the user-provided override. Anyone passing --base main would expect it to control the diff comparison base, but it silently has no effect.
Additional Locations (2)
|
|
||
| function filesWithPattern(pattern) { | ||
| return changedFiles.filter(f => fileDiffs[f] && pattern.test(fileDiffs[f])) | ||
| } |
There was a problem hiding this comment.
Renamed files missed by pattern-based subagent routing
Low Severity
parseDiffByFile indexes diff content by the old filename (extracted from a/... in diff --git a/old b/new), but changedFiles from git diff --name-only uses the new filename. When filesWithPattern looks up fileDiffs[f] for a renamed file, the key mismatch causes undefined, so the file is silently excluded from pattern-based subagents like review-async, review-state, review-cleaners, and review-errors.
| process.env.GH_TOKEN = b64 | ||
| } | ||
| } catch (_) {} | ||
| } |
There was a problem hiding this comment.
Duplicated ensureGhToken function across two scripts
Low Severity
The ensureGhToken function is identically duplicated between review-prep.js and review-submit.js. This increases maintenance burden — a fix to the token resolution logic (e.g., supporting a new token format) would need to be applied to both copies independently.
Additional Locations (1)
Triggered by team rule: Use review sub agents
7 participants
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneDescription
Add cursor commands and subagents for pull request reviews.
Also add cursor commands and skills for executing a development plan with proper debug skills using Xcode and mobile MCP servers.
Add human readable doc that outlines all agent functionalities
To reviewers of this pull request, please start the review with the agent-setup.md document.
Note
Low Risk
Primarily adds new tooling/docs under
.cursor/with no runtime product impact; main risk is workflow friction or incorrect automation behavior when used (git/gh commands, file writes).Overview
Introduces a suite of Cursor review subagents (async, cleaners, errors, React, state, strings, etc.) that codify Edge-specific review conventions and provide structured guidance for scanning diffs.
Adds automation commands/skills for common workflows: executing plan docs (
codeit), scraping PR bugs/reviews into new rules (scrapebugs,scraperev), packaging local dependency tarballs (package-deps), interactive pending GitHub reviews viagh(gh-review), and end-to-end review preparation/reporting/submission (review-codescripts) including subagent selection heuristics.Documents the end-user setup and usage in
docs/agent-setup.md, including MCP server installation and how to run the new workflows.Written by Cursor Bugbot for commit 063417e. This will update automatically on new commits. Configure here.