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
| | comments, documentation, JSDoc, TODO | review-comments.md | | ||
| | commit message, git, rebase, PR structure | review-pr.md | | ||
| | lstrings, en_US.json, localization, hardcoded string | review-strings.md | | ||
| | naming, dead code, unused, organization | review-code-quality.md | |
There was a problem hiding this comment.
Missing review-tests entry in scraperev categorization table
Low Severity
The categorization table in scraperev.md that routes PR review comments to review agent files is missing an entry for review-tests.md. The revpr.md command lists review-tests as one of the subagents to launch (line 152), but scraperev has no keyword mapping for test-related patterns (like "test", "coverage", "unit test", "mock", "testability"). This means test-related PR feedback will never be captured as rules in review-tests.md during the scraperev workflow.
Additional Locations (1)
Triggered by team rule: Use review sub agents
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
Or push these changes by commenting: Preview (c7f8ff273c)diff --git a/.cursor/commands/scraperev.md b/.cursor/commands/scraperev.md
--- a/.cursor/commands/scraperev.md
+++ b/.cursor/commands/scraperev.md
@@ -201,6 +201,7 @@
| commit message, git, rebase, PR structure | review-pr.md |
| lstrings, en_US.json, localization, hardcoded string | review-strings.md |
| naming, dead code, unused, organization | review-code-quality.md |
+| test, tests, unit test, mock, coverage, testability, jest, spec | review-tests.md |
### Step 6: Check for Duplicates
@@ -362,3 +363,10 @@
- Delete unused code
- Inline parameters
- Use existing helpers
+
+### review-tests.md
+- Unit test coverage
+- Testability patterns
+- Dependency injection for testing
+- Mock and stub usage
+- Test organization |
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.
| ```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.
|
|
||
| ## 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) {}|
|
||
| ## 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.
|
|
||
| ## 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
| }) | ||
| ``` | ||
|
|
||
| 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.
| 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.
| ## 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
|
|
||
| Use the GitHub MCP server to fetch reviewer comments from the PR: | ||
|
|
||
| 1. Read the tool schema first: `/Users/paul/.cursor/projects/Users-paul-git-edge-react-gui/mcps/user-github/tools/` |
There was a problem hiding this comment.
Hardcoded developer-specific path in skill instructions
High Severity
The path /Users/paul/.cursor/projects/Users-paul-git-edge-react-gui/mcps/user-github/tools/ is a developer-specific absolute path that only exists on one machine. Any other developer (or AI agent) following this skill will fail at this step since the path won't exist on their system.
| 8. Resolve any conflicts | ||
| 9. Update the branch to the new history: `git checkout -B <branch-name>` | ||
|
|
||
| Repeat for each fix item. |
There was a problem hiding this comment.
Multi-fixup workflow uses stale commit hashes
High Severity
When processing multiple fixes, each iteration rewrites git history (via cherry-pick), making all subsequent FIXUPHASH values from the original plan stale. The second iteration checks out the original commit hash and cherry-picks a range spanning two divergent DAG branches, pulling in duplicate fixup commits and causing conflicts. This workflow only works correctly for a single fix item.
| "edge-core-js": "./edge-core-js-2.38.4-20260202T0406.tgz" | ||
| ``` | ||
|
|
||
| Repeat for all dependency repos mentioned in the prompt. |
There was a problem hiding this comment.
Missing yarn install step leaves stale dependency in node_modules
Medium Severity
The packdep command updates package.json to reference the local tarball but never runs yarn or yarn install to actually install it. Without this step, node_modules still contains the old published version of the dependency. The AI agent (or developer) would think they're testing new local changes, but the app silently runs against stale code from the previous version, producing misleading test results.
|
Bugbot Autofix prepared fixes for 3 of the 3 bugs found in the latest run.
Or push these changes by commenting: Preview (22d4ab9bbd)diff --git a/.cursor/commands/packdep.md b/.cursor/commands/packdep.md
--- a/.cursor/commands/packdep.md
+++ b/.cursor/commands/packdep.md
@@ -14,5 +14,6 @@
```json
"edge-core-js": "./edge-core-js-2.38.4-20260202T0406.tgz"
```
+6. Run `yarn` in the edge-react-gui directory to install the local tarball into `node_modules`
Repeat for all dependency repos mentioned in the prompt.
diff --git a/.cursor/skills/fix-pr/SKILL.md b/.cursor/skills/fix-pr/SKILL.md
--- a/.cursor/skills/fix-pr/SKILL.md
+++ b/.cursor/skills/fix-pr/SKILL.md
@@ -20,9 +20,8 @@
Use the GitHub MCP server to fetch reviewer comments from the PR:
-1. Read the tool schema first: `/Users/paul/.cursor/projects/Users-paul-git-edge-react-gui/mcps/user-github/tools/`
-2. Fetch PR details and all review comments
-3. Look for:
+1. Fetch PR details and all review comments using the GitHub MCP tools
+2. Look for:
- Line-level review comments
- General PR comments with actionable feedback
- Requested changes from reviewers
@@ -93,16 +92,17 @@
For each fix in the plan:
1. Save the current branch head hash as `BRANCHHEAD`
-2. Save the hash of the commit to fix as `FIXUPHASH`
-3. Check out the commit needing changes: `git checkout ${FIXUPHASH}`
-4. Make the necessary code changes (edit files, stage them)
-5. Test changes with `yarn precommit` and fix any failures
-6. Stage and create the fixup commit: `git add -A && git commit --fixup HEAD --no-verify`
-7. Cherry-pick remaining commits: `git cherry-pick "${FIXUPHASH}..${BRANCHHEAD}"`
-8. Resolve any conflicts
-9. Update the branch to the new history: `git checkout -B <branch-name>`
+2. Re-identify the commit to fix by examining `git log --oneline` and matching the commit message or content from the plan (original commit hashes become stale after history rewrites)
+3. Save the newly identified hash as `FIXUPHASH`
+4. Check out the commit needing changes: `git checkout ${FIXUPHASH}`
+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}"`
+9. Resolve any conflicts
+10. Update the branch to the new history: `git checkout -B <branch-name>`
-Repeat for each fix item.
+Repeat for each fix item. **Important**: After each iteration, the branch history is rewritten, so commit hashes from the original plan are no longer valid. Always re-identify commits using `git log` at the start of each iteration.
## Pushing Changes to GitHub |
|
|
||
| ## 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. |
| 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
.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.
|
|
||
| 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
| 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.
| 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.
|
|
||
| ## 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
| **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).
4 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
Adds Cursor configuration/docs only; no runtime application logic changes, so risk is limited to developer workflow confusion if instructions are incorrect.
Overview
Adds a set of Cursor review subagents under
.cursor/agents/that codify repo conventions for async patterns, cleaners/data validation, errors, React, state, strings, tests, servers, comments, PR hygiene, plus areview-repoorchestrator for routing by repository.Introduces several Cursor commands and skills under
.cursor/commands/and.cursor/skills/for automating common workflows (plan execution with build/test loops, iOS simulator debugging via MCP, PR review/fix flows, dependency packing, and scraping PRs/reviews into new agent rules). Also addsdocs/agent-setup.mdto document MCP setup and available commands.Written by Cursor Bugbot for commit 73a3d4b. This will update automatically on new commits. Configure here.