docs: single tool-neutral rules.md; CLAUDE.md + AGENTS.md as thin pointers - #607
Conversation
… pointers Consolidate all repo rules into one canonical rules.md so the standard is identical whatever agent reads it (Claude/Codex/etc.). CLAUDE.md and AGENTS.md now just reference it (AGENTS.md was also stale — claimed pre-commit gates + unit+integration tests, contradicting the pre-push gates and lean one-integration-test doctrine). Adds an On-device testing & verification playbook that codifies the WDA(iOS)/adb(Android) direct-drive flow, coordinate math, fix-onto-device build paths, log-pull, the both-devices-before-merge gate, and the macOS/permission-dialog/flaky-gate gotchas.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
📝 WalkthroughWalkthroughThe PR centralizes repository guidance in ChangesRepository rules consolidation
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rules.md`:
- Line 147: Update the pre-push workflow documentation in rules.md, including
the referenced testing doctrine and push checklist sections, to clearly
distinguish hook-enforced checks from manually required checks. Align the
documented Jest scope with the actual workflow: identify where jest
--findRelatedTests runs automatically and where npm test/full-suite execution is
explicitly required, avoiding ambiguous or duplicated guidance.
- Around line 27-28: Update the device selection command around DEVICE so it
does not require tunnelState to equal "connected"; select a reachable target
independently of tunnel state and preserve support for devices reported as
"disconnected". Also provide a fallback allowing the operator to supply the
CoreDevice identifier when automatic selection cannot find a device, avoiding
StopIteration.
- Line 55: Reconcile the commit policy in the rules around the “safe restore
point” guidance and the red-test instructions at the later TDD section. Define a
single consistent rule: permit failing commits only as isolated, intentional TDD
steps, while requiring the branch to be green before pushing and prohibiting
knowingly broken unrelated work.
- Around line 14-16: Update the logging guidance in rules.md to remove the claim
that npx react-native log-ios is simulator-only. Describe it according to the
CLI behavior as an iOS syslog tail, while retaining the distinction that
JS/React Native console logs do not reach the physical device syslog and
preserving the dev-only persistent file sink instructions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e6c826a-b8bf-424f-aa6f-a87a4cf5e33d
📒 Files selected for processing (3)
AGENTS.mdCLAUDE.mdrules.md
| **RN 0.83 moved JS `console.log` off the Metro terminal into React Native DevTools, and RN's console never reaches the iOS device syslog.** So `metro` stdout, `idevicesyslog`, and `npx react-native log-ios` (simulator-only) all capture NOTHING from a physical device. Do not waste time tailing Metro for app logs. | ||
|
|
||
| Instead, a **dev-only persistent file sink** (`src/utils/debugLogFile.ts`, wired in `App.tsx` behind `__DEV__`) mirrors every `logger.*` line - which is where ALL the state-machine traces go (`[TTS-SM]`, `[GEN-SM]`, `[MODEL-SM]`, `[DL-SM]`, `[ROUTE-SM]`, `[IMG-SM]`, `[MEM-SM]`, `[FAIL-SM]`) - into a file in the app container. Pull it over the cable to read the real trace: |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "Starts iOS device syslog tail|log-ios" \
node_modules/@react-native-community/cli-platform-ios \
node_modules/@react-native-community/cliRepository: off-grid-ai/OGAM
Length of output: 428
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- rules.md relevant section ---\n'
sed -n '1,25p' rules.md
printf '\n--- `@react-native-community/cli-platform-ios` log-ios README section ---\n'
sed -n '180,210p' node_modules/@react-native-community/cli-platform-ios/README.md 2>/dev/null || trueRepository: off-grid-ai/OGAM
Length of output: 245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- rules.md relevant section ---'
sed -n '1,25p' rules.md
printf '%s\n' ''
printf '%s\n' '--- `@react-native-community/cli-platform-ios` log-ios README section ---'
sed -n '180,210p' node_modules/@react-native-community/cli-platform-ios/README.md 2>/dev/null || trueRepository: off-grid-ai/OGAM
Length of output: 2739
Do not label npx react-native log-ios as simulator-only.
The React Native CLI docs describe log-ios as the iOS device syslog tail; simulator interaction is only via the interactive option. Keep the guidance focused on JS/React Native console logs not reaching the device syslog so native physical-device diagnostics are not discarded.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rules.md` around lines 14 - 16, Update the logging guidance in rules.md to
remove the claim that npx react-native log-ios is simulator-only. Describe it
according to the CLI behavior as an iOS syslog tail, while retaining the
distinction that JS/React Native console logs do not reach the physical device
syslog and preserving the dev-only persistent file sink instructions.
| xcrun devicectl list devices --json-output /tmp/devs.json >/dev/null 2>&1 | ||
| DEVICE=$(python3 -c "import json;ds=json.load(open('/tmp/devs.json'))['result']['devices'];print(next(d['hardwareProperties']['udid'] for d in ds if d.get('connectionProperties',{}).get('tunnelState')=='connected'))") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle reachable devices whose tunnel state is disconnected.
This script exits with StopIteration whenever no device reports tunnelState == "connected", but the later guidance explicitly says "disconnected" does not mean unreachable. Select or accept the target device independently of tunnel state, or provide a fallback that lets the operator pass the CoreDevice identifier.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rules.md` around lines 27 - 28, Update the device selection command around
DEVICE so it does not require tunnelState to equal "connected"; select a
reachable target independently of tunnel state and preserve support for devices
reported as "disconnected". Also provide a fallback allowing the operator to
supply the CoreDevice identifier when automatic selection cannot find a device,
avoiding StopIteration.
|
|
||
| - **Commit each cohesive step as soon as it is green** (typecheck + the relevant tests pass), with a real per-concern message. A refactor done in slices commits after each slice, not at the end. | ||
| - **Never leave a large uncommitted working tree across a risky/long operation.** If you are about to start something big, commit what already works first so there is a clean restore point. | ||
| - **Every commit is a safe restore point:** it must be behavior-neutral-or-better and pass the gates for the files it touches. Do not commit a knowingly-broken tree; if mid-refactor is unavoidably broken, finish to green before committing (or stash), never push broken. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Resolve the red-commit contradiction.
Lines 55 and 155-156 give opposite rules: one forbids committing a knowingly broken tree, while the other explicitly instructs committing a failing test red. Define one policy, such as allowing red commits only for isolated TDD steps while forbidding pushes of a red branch.
Also applies to: 155-156
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rules.md` at line 55, Reconcile the commit policy in the rules around the
“safe restore point” guidance and the red-test instructions at the later TDD
section. Define a single consistent rule: permit failing commits only as
isolated, intentional TDD steps, while requiring the branch to be green before
pushing and prohibiting knowingly broken unrelated work.
|
|
||
| | Pushed file type | Checks that run automatically (pre-push) | | ||
| |---|---| | ||
| | `.ts` / `.tsx` / `.js` / `.jsx` | eslint, `tsc --noEmit`, `jest --findRelatedTests`, `npm run depcruise` | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Align the documented test scope with the actual workflow.
The pre-push table runs jest --findRelatedTests, while the testing doctrine says the full suite runs at pre-push and the push checklist separately runs npm test. State clearly which checks are hook-enforced and which are manually required before pushing; otherwise agents may incorrectly skip or duplicate the full suite.
Also applies to: 165-166, 176-176
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rules.md` at line 147, Update the pre-push workflow documentation in
rules.md, including the referenced testing doctrine and push checklist sections,
to clearly distinguish hook-enforced checks from manually required checks. Align
the documented Jest scope with the actual workflow: identify where jest
--findRelatedTests runs automatically and where npm test/full-suite execution is
explicitly required, avoiding ambiguous or duplicated guidance.
1 participant
What
Consolidate all repo rules into one canonical
rules.md(tool-neutral — same standard whether the agent is Claude, Codex, or anything else).CLAUDE.mdandAGENTS.mdbecome thin pointers to it.Why
AGENTS.mdhad drifted — it claimed pre-commit gates and "write unit and integration tests," which contradicts the actual pre-push gates and the lean "one rendered integration test per fix" doctrine inCLAUDE.md. Consolidating removes the contradiction.New content
Adds an On-device testing & verification playbook codifying hard-won learnings so we never re-derive them:
launchWdamechanism only, not the Provit journey engine) and Android viaadb; prefer accessibility-id/element taps; screenshot after every tap.scripts/ios-device.sh; Android loads from Metro viaadb reverse).devicectl copy, Androidrun-as), the lazy file-sink flush, and the[WIRE-*]markers.timeouton macOS,tunnelState:disconnected≠ unreachable, permission dialogs eating taps, keep device unlocked, flaky pre-push jest gate → retry.No behavior/code changes.
Summary by CodeRabbit