docs(agents): add repo-review skill for whole-repo audits - #10
Conversation
The worklog-reviewer agent reviews a diff against the repo's invariants. This skill covers the defect classes a per-diff review structurally cannot see: guard-allowlist decay, cross-file drift, dead code, untested modules, doc/code divergence, and aggregate security posture. Six passes (guard integrity, layering, offline-first contract, UI surface, test quality, security/doc truth), run as parallel subagents since the repo does not fit one context at review depth. Gates run first so findings sit on top of real results rather than intuition. Includes a known-non-findings list so each run stops re-reporting settled facts, and evidence rules to hold down the false positives a 160-file sweep otherwise produces. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MJ5tDBkqn1LoYeojaWUKip
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdded a ChangesRepository Review Workflow
Estimated code review effort: 2 (Simple) | ~10 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 |
allowed-tools is a restriction, so naming only Task silently strips the fan-out capability on harnesses that expose the subagent tool as Agent — and fanning the six passes out is the skill's core mechanic. List both and keep the prose tool-agnostic. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MJ5tDBkqn1LoYeojaWUKip
The skill invented docs/reviews/, but docs/superpowers/reviews/ already holds this artifact class — a second home for the same thing is the cross-file drift Pass B exists to catch. Untracked .superpowers/ stays the home for transient agent output, matching how the repo already splits the two. Also require findings to be resolved (fixed with a SHA, declined with a reason, or filed as an issue) before a report is committed. A raw finding list goes stale and misleads; a record of what was decided does not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MJ5tDBkqn1LoYeojaWUKip
Ran Pass A against 28c410d. It found two real issues in the repo, and four in the skill: - Step 0 said to run the gates but not to check their exit status. Piping a gate into tail returns tail's status, so a failed verify read as a pass on the calibration run. Also handles an empty node_modules, which is what a fresh container actually looks like, and requires CI fallbacks to be labelled second-hand. - The native-module rule ("anything native-only beyond these four") was wrong and produced a false positive on expo-secure-store. Native-only means "does not resolve in the web bundle", not "is a React Native package"; a static import inside a Platform.OS branch is not by itself a finding. Also stops quoting the array's contents, which go stale. - Pass E hardcoded an untested-module count while the Output section warns against exactly that. Now computes it, with the correct -f test: the obvious `ls a b` form exits non-zero when either operand is missing and reports every file as untested. - Four adjudications from this run added to the non-findings list so the next run doesn't re-derive them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MJ5tDBkqn1LoYeojaWUKip
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b337e588e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| `expo-crypto` and `@react-native-async-storage/async-storage` are all imported | ||
| from non-`.native.` files today and all ship web builds, so they are correctly | ||
| *absent* from the array. Before reporting one, check its import sites and | ||
| confirm the `web-export` job is red. A static import inside a `Platform.OS` |
There was a problem hiding this comment.
When a newly added native-only dependency is correctly imported only from a .native file but omitted from NATIVE_ONLY_MODULES, the web export remains green; requiring a red web-export job therefore suppresses the guard-integrity finding described just above. That leaves the dependency unregistered and allows a later non-native import to bypass the grep guard, so native-only status should be established independently of whether the current web graph already imports it.
Useful? React with 👍 / 👎.
| - **Every write goes through the queue.** Enumerate every mutating call site in | ||
| `src/data/` and `src/sync/` and account for each. A read path that writes, or | ||
| a repo method bypassing `mutationQueue`, is the highest-value finding here. |
There was a problem hiding this comment.
On every current audit, enumerating all mutations in src/data/ includes intentionally direct writes such as the online-only web repository RPCs and createProject.ts's documented server-only insert. Declaring any repository method that bypasses mutationQueue the highest-value finding conflicts with those supported paths and can produce false HIGH findings; this check needs to be scoped to native, offline-capable report mutations and explicitly exempt documented online-only operations.
Useful? React with 👍 / 👎.
| ```bash | ||
| ls node_modules >/dev/null 2>&1 || echo "DEPS ABSENT — gates cannot run" | ||
| npm run verify; echo "verify=$?" # typecheck + format:check + lint + test | ||
| npm run check:web; echo "check:web=$?" # the real platform-split check |
There was a problem hiding this comment.
In a normal checkout without the ignored .env, this command fails because src/supabase/client.ts throws when the two Supabase variables are absent, rather than because the platform split is broken. The inspected web-export CI job supplies placeholder values for exactly this reason, so Step 0 should do the same or classify missing env separately; otherwise a runnable, healthy web gate is reported as failed or NOT RUN.
Useful? React with 👍 / 👎.
| ls node_modules >/dev/null 2>&1 || echo "DEPS ABSENT — gates cannot run" | ||
| npm run verify; echo "verify=$?" # typecheck + format:check + lint + test | ||
| npm run check:web; echo "check:web=$?" # the real platform-split check | ||
| npm run check:parity; echo "parity=$?" # regenerates from ../jobsight-backend |
There was a problem hiding this comment.
When the sibling backend has schema drift, npm run check:parity first rewrites the tracked src/db/serverColumns.generated.json and only then fails on git diff. Thus the read-only review leaves the caller's worktree modified precisely when it discovers a parity issue, contradicting the skill's “Change nothing” contract; the gate should compare through a temporary copy or restore the original snapshot after capturing the result.
Useful? React with 👍 / 👎.
| new screens with no testIDs at all. | ||
| - **Dark mode and reduced motion.** Every surface renders in both themes; | ||
| animations respect `useReducedMotion`. | ||
| - **Accessibility.** Touch targets ≥44pt, labels on icon-only controls, |
There was a problem hiding this comment.
The repository's product requirement is at least 48×48 px (docs/PRD.md AC-T1 and the architecture work plan), not 44pt. As written, the UI pass accepts controls sized 44–47 px even though they violate the field/glove usability acceptance criterion, so the audit can miss a concrete spec regression.
Useful? React with 👍 / 👎.
| while the actual checks are unrun is guessing. | ||
|
|
||
| ```bash | ||
| ls node_modules >/dev/null 2>&1 || echo "DEPS ABSENT — gates cannot run" |
There was a problem hiding this comment.
When node_modules/ exists but is empty—the fresh-container case called out below—ls node_modules exits successfully, so the diagnostic is not emitted and dependency failures are presented alongside genuine gate failures. Test for at least one directory entry (or a required package) rather than directory existence so this environment is reliably classified as NOT RUN.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.claude/skills/repo-review/SKILL.md (1)
190-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the hard-coded untested-module count.
The procedure correctly says to compute the list, but then embeds a time-sensitive “~35 files” claim. Derive the count from the command output or omit it so the guidance does not become stale.
🤖 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 @.claude/skills/repo-review/SKILL.md around lines 190 - 205, Remove the time-sensitive “~35 files” statement from the Untested modules guidance, and retain the command-based instruction to compute the current list. Do not replace it with another hard-coded count; have the procedure derive or omit the count while preserving the ranking and priority guidance.
🤖 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 @.claude/skills/repo-review/SKILL.md:
- Line 4: Update the repo-review skill’s allowed-tools list to remove Write,
preserving the read-only audit contract. Adjust the parity command instructions
around the referenced execution section so it runs in a disposable copy/worktree
or uses a non-writing mode, preventing regeneration from modifying the
checked-in schema snapshot.
- Around line 38-43: Update the dependency-availability check in the gate
command block to detect whether node_modules contains installed dependencies,
not merely whether the directory exists. Treat an absent or empty node_modules
directory as “DEPS ABSENT — gates cannot run,” while preserving the existing
verify, check:web, and check:parity commands and status reporting.
- Around line 231-232: Update the Dependencies guidance in the dependency pin
validation section to avoid unversioned `npx expo-doctor`; direct reviewers to
the repository’s locally installed `node_modules/.bin/expo-doctor` or specify
the Expo SDK 54-compatible package version, preserving the existing validation
intent.
---
Nitpick comments:
In @.claude/skills/repo-review/SKILL.md:
- Around line 190-205: Remove the time-sensitive “~35 files” statement from the
Untested modules guidance, and retain the command-based instruction to compute
the current list. Do not replace it with another hard-coded count; have the
procedure derive or omit the count while preserving the ranking and priority
guidance.
🪄 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: 5d8e86cd-550b-48af-b63c-edb14c4a07f6
📒 Files selected for processing (1)
.claude/skills/repo-review/SKILL.md
Nine findings across both bots; all verified against the repo before acting. Gate setup (Step 0): - `ls node_modules` exits 0 on an empty directory, which is exactly the fresh-container case the check exists to catch. Tests for entries now. - check:web failed on missing Supabase env vars rather than on the platform split. Supplies placeholders, as ci.yml already does. - check:parity regenerates a tracked snapshot before diffing, so it left the worktree dirty precisely when it found drift — against the skill's own change-nothing contract. Restores and verifies afterwards. Pass A: a green web-export no longer clears a missing NATIVE_ONLY_MODULES entry. A native dep imported only from .native. files keeps the export green and still must be registered, since the array exists to catch the next import. Splits the two questions apart and keeps the expo-secure-store adjudication, which holds for a different reason than first written: the app imports it from a shared file by design, so it cannot be in the array at all. Pass C: scoped the queue audit to native offline-capable report mutations. supabaseRepo's online-only RPCs and createProject's documented server-only insert are supported paths; flagging them is a false HIGH. Pass D: touch-target floor is this repo's 48x48 px (PRD AC-T1, gloved field use), not the 44pt platform default, plus AC-T2 spacing. Pass F: stop recommending unpinned `npx expo-doctor` — not a dependency here, so it downloads and runs whatever is current. Points at CI's deps job. Also drops a hardcoded untested-module count that the Output section's own staleness rule forbids, and states why the skill carries Write but not Edit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MJ5tDBkqn1LoYeojaWUKip
…list The Codex fix in 90c5a02 corrected Pass A but left the non-findings entry still asserting the rule it had just overturned: "all resolve on web, web-export green ... re-open only if web-export fails". Pass A now says a green export never clears a missing registration, so the two sections contradicted each other — the same-concept-implemented-two-ways drift this skill audits for, inside the skill. The adjudication itself stands; only its reasoning was wrong. These modules are imported from shared files behind a Platform.OS branch with a real web fallback, so the array cannot contain them at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MJ5tDBkqn1LoYeojaWUKip
…ep 0 The restore added in 90c5a02 used an unconditional `git checkout --`, which silently destroys a pre-existing uncommitted edit to serverColumns.generated.json — and mid-schema-change is exactly when that file is dirty. Restoring the worktree is only safe when the snapshot was clean going in. Guards on both the worktree and the index, and skips the gate with NOT RUN when the file is already modified. A review does not trade someone's uncommitted work for a gate result. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MJ5tDBkqn1LoYeojaWUKip
Ran Pass C against the repo. It produced four false positives and zero true findings — the judgment-heavy pass behaving exactly as feared. Both causes were the pass asserting a structure the code does not have: - The mutation-kind matrix flagged add_photo, update_photo_meta and remove_photo as missing handlers. They are deliberately staged: rpcMap throws 'photo kinds are M5' and rpcMap.test asserts that throw. An absence that is explicit, milestone-tagged and tested is a plan, not a defect. The real finding is a kind with no handler and no throw. - The matrix also required "a conflict rule" per kind. There is no per-kind conflict table and shouldn't be: conflict.ts is generic last-writer-wins on updated_at over MergeableItem. A reviewer following the old text would hunt for a structure that does not exist and report its absence. Replaced with the checks that do matter — every pulled row through resolveItem, _dirty clearing when the server wins, and _dirty surviving only while something queued still targets the row. Same family as the false-HIGH Codex caught in this pass, which suggests the disease is any bullet phrased "every X has a Y". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MJ5tDBkqn1LoYeojaWUKip
Pass C's false positives came from bullets asserting a structure without checking the code has it. Swept B, D and F for the same shape. B: two problems. The repository-seam bullet hardcoded the legitimate-importer list (still accurate — six files — but it rots exactly like the NATIVE_ONLY list removed earlier), so it now carries the grep that computes it. And "confirm a web counterpart exists" would have flagged all seven .native modules that have no .web sibling; that is the normal case, since the web graph never reaches them. The pairing that must exist is at the platformRepo seam, not per file. F: the open-decisions list was hardcoded against a section that shrinks as decisions get made. Points at the section instead. D needed nothing beyond the 48px fix already made — its bullets ask questions about named files rather than asserting structure, which is why Pass A held up under scrutiny too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MJ5tDBkqn1LoYeojaWUKip
First full six-pass run (against 8f5cccd): 38 raw findings, 35 after dedupe, 1 CRITICAL. The calibrated passes behaved — Pass C, which produced four false positives and zero true findings before calibration, this time listed the photo kinds and the row-level conflict model as deliberate and found a silent data-loss bug instead. What the run taught the skill: Pass A: every allowlist was honest and four guard MECHANISMS were broken. That is where the findings are, so the pass now audits mechanisms explicitly — what a detector's regex cannot see (bare side-effect imports), whether an assertion can be satisfied by a prose comment rather than the code, whether a generator can represent every form of its input (the parity generator cannot see RENAME COLUMN), whether a coverage pin on a data-only file is falsifiable at all, and whether a guard proves it scanned anything. Those five examples are live defects, not hypotheticals, so they are filed as #14 and #15 and labelled as shapes to hunt rather than settled facts. An example left in a skill without that note quietly becomes a suppression. Evidence discipline: added cleared-vs-vacuous. Several Pass C checks passed only because the pull path has no callers yet; reporting those as clean implies coverage that does not exist. Merging: the step the run exercised for the first time, and the skill said almost nothing about it. Now requires verifying every CRITICAL and HIGH at source before publishing, deduping across passes as corroboration, arbitrating severity disagreements via the repo's own rubric rather than the reviewer's taste, and ranking rather than concatenating. Step 0: run npm ci when deps are absent instead of settling for second-hand CI results — that is what made three real gate results possible this run. Output: file one issue per CRITICAL/HIGH and group the rest by what a single fix would touch. Thirty-five issues is a tracker nobody reads; five covered it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MJ5tDBkqn1LoYeojaWUKip
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.claude/skills/repo-review/SKILL.md (2)
33-36: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the report template consistent with the allowed finding set.
The workflow permits findings that a diff review could have caught when a missing guard allowed the defect through. The output template then requires every finding to explain why a diff review could not have caught it. Replace that requirement with: explain the whole-repository value, and identify the missing guard when the underlying defect was diff-visible.
Also applies to: 504-506
🤖 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 @.claude/skills/repo-review/SKILL.md around lines 33 - 36, Update the finding-reporting guidance in the repository review workflow so each finding explains its whole-repository value, and identifies the missing guard when the underlying defect was visible in the diff. Apply the same wording consistently to the corresponding report template section around the other referenced occurrence.
271-274: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAllow migration-based schema compatibility.
A schema or payload change does not require a both-shapes read path when a migration or backfill converts existing data before reads. The current rule labels every such change as device-bricking and will produce false findings. Require either migration/backfill coverage or explicit dual-shape reading, then verify upgrade ordering.
🤖 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 @.claude/skills/repo-review/SKILL.md around lines 271 - 274, Update the “OTA shape compatibility” guidance in the repo-review skill to accept either migration/backfill coverage that converts existing data before reads or an explicit dual-shape read path. Require reviewers to verify that the migration or backfill runs before dependent reads, while retaining the device-bricking finding for changes lacking both forms of compatibility.
🤖 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 @.claude/skills/repo-review/SKILL.md:
- Around line 455-457: Update the calibration guidance around the validated run
to remove the finding-count quality gate and its implications that low counts
indicate shallow work or high counts indicate failed verification. Retain the
run statistics solely as historical context, and state that review conclusions
must be based on verified evidence rather than finding counts.
- Around line 390-400: Update the documented “known non-findings” entries for
dependencies and pull-path primitives to require revalidation during every
audit. Preserve the historical examples and staging rationale, but instruct
auditors to freshly verify web resolution, import sites, callers, and test
coverage before suppressing findings.
---
Outside diff comments:
In @.claude/skills/repo-review/SKILL.md:
- Around line 33-36: Update the finding-reporting guidance in the repository
review workflow so each finding explains its whole-repository value, and
identifies the missing guard when the underlying defect was visible in the diff.
Apply the same wording consistently to the corresponding report template section
around the other referenced occurrence.
- Around line 271-274: Update the “OTA shape compatibility” guidance in the
repo-review skill to accept either migration/backfill coverage that converts
existing data before reads or an explicit dual-shape read path. Require
reviewers to verify that the migration or backfill runs before dependent reads,
while retaining the device-bricking finding for changes lacking both forms of
compatibility.
🪄 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: 8326f5b4-f50d-4386-aa23-f491bfa784e9
📒 Files selected for processing (1)
.claude/skills/repo-review/SKILL.md
Four valid findings on the content added in 50452d3, two of which are the same suppression-rot disease this skill exists to catch, in my own additions. - The "expect roughly this shape" line set a finding-count expectation, which pressures a run toward padding or trimming and contradicts the skill's own "reporting nothing is a perfectly good answer". Run stats are now explicitly historical scale context, with the quality bar restated as evidence. - The known-non-findings entries carry dated bases ("verified in node_modules on 2026-07-30") but read as permanent immunity. A dependency bump or a new caller can invalidate the basis while the entry still says don't report. They are now adjudications requiring a cheap re-check of the stated basis, with entries that rest on settled design exempted. - The OTA rule demanded a both-shapes read path, which would flag a correctly migrated change — MIGRATIONS[2] is exactly that and is fine. Now accepts either a migration/backfill that converts before dependent reads or a dual-shape reader, and requires checking the ordering. Keeps the sharper case: a migration rewrites table rows but not payloads already serialized into the queue, so a payload change still needs an explicit queue rewrite or a both-shapes handler. - The report template demanded every finding explain why a diff review could not have caught it, while the intro allows diff-visible findings when a guard is missing. Template now asks what made it a whole-repo find, and for the missing guard when the defect was diff-visible. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MJ5tDBkqn1LoYeojaWUKip
First full run of the repo-review skill. 38 raw findings, 35 after dedupe, across six parallel passes. Committed per the skill's own rule: a report goes into docs/superpowers/reviews/ only once its findings are resolved, and all 35 are now filed as issues #11-#24. Recorded as a decision record rather than a to-do list — findings map to issue numbers, so the document ages as history instead of rotting into a stale backlog. Two of the findings are silent, unrecoverable data loss (#12, #13). The review also states what it could not see: check:parity did not run, so nothing server-side was verified, and the pull-path checks were vacuous rather than clean because those modules have no callers yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MJ5tDBkqn1LoYeojaWUKip
CodeRabbit flagged the report template for demanding every finding explain why a diff review could not have caught it, while the workflow allows diff-visible findings when a guard is missing. I fixed the template and missed the source of the inconsistency, which was the intro: "every finding must be one a reviewer looking at a single commit could not have found" immediately followed by "if it would have been caught ... it still counts". The bar is whole-repo value, not literal invisibility to a diff reviewer. A defect that shipped despite being diff-visible still counts, and the guard that should have stopped it is a second finding. What does not belong is a defect an ordinary diff review would routinely catch and no guard was meant to prevent — that is worklog-reviewer's job on the next PR. Also makes the DEPS ABSENT hint actionable rather than contradicting the npm ci guidance twenty lines below it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MJ5tDBkqn1LoYeojaWUKip
2 participants
What
Adds
.claude/skills/repo-review/SKILL.md— a skill for auditing the entire repo, as opposed to theworklog-revieweragent, which reviews a diff against the repo's invariants. Also addsdocs/superpowers/reviews/2026-07-30-repo-review.md, the record of the first full run.The skill deliberately does not restate
worklog-reviewer's invariant list; it defers to that file as the authority and targets the defect classes a per-diff review structurally cannot see: guard decay, cross-file drift, dead code, whole untested directories, doc↔code divergence, and aggregate security posture.Structure: gates run first (
verify,check:web,check:parity) so findings sit on top of real results; then six passes — guard integrity, layering/boundaries, the offline-first contract, UI surface, test-suite quality, and security/config/doc truth — dispatched as parallel subagents, since ~160 source files do not fit one context at review depth. The merge, dedupe and severity arbitration are the parent's job and are specified.Reports go to
docs/superpowers/reviews/alongside the existing Phase 4 review, and only once their findings are resolved — fixed with a SHA, declined with a reason, or filed as an issue. The raw sweep stays in the scratchpad.It has been run, and the run is what shaped it
Full six-pass run against
8f5cccd: 38 raw findings → 35 after dedupe, 1 CRITICAL. All 35 are filed as #11–#24; the record is indocs/superpowers/reviews/2026-07-30-repo-review.md.Two findings are silent, unrecoverable data loss and are worth looking at independently of this PR:
discardParked'screate_reportcascade fires on a pending mutation. Tap Retry then Discard during the drain and the report plus its section edits land on the server and are deleted from the device, with no pull path to recover them and no signal to the user.didFallBackToOnlineOnly()has no consumer. If SQLite fails to open, the app runs online-only while the pill says "All saved to the cloud" and the toast promises a retry that cannot happen.Three earlier calibration rounds and this run each changed the skill:
tailreturns tail's status).Milestone / plan task
None — agent tooling, not a milestone task.
Gates
npm run verify— PASS, run locally afternpm ci: 50 suites, 456 tests, every coverage pin met. Green in CI on every commit here.npm run check:web— PASS, run locally with placeholder Supabase env.npm run check:parity— NOT RUN:../jobsight-backendis absent from this container. Recorded as NOT RUN in the review rather than inferred from the committed snapshot.docs/architecture/00-README.md? → no. The run found one silently implemented (Open decision R1: the index says it needs a decision, 06-sync-mappings says it is approved, and the SQLite schema already ships it #17) and filed it rather than deciding it.Open decisions touched
None decided here. #17 reports that R1 is listed as open in
00-README.md, called approved in06-sync-mappings.md, and already shipped inSCHEMA_V1— labelledready-for-humanbecause the resolution is a product call.Adversarial review
No source change to review, so
worklog-reviewerwas not run. The skill was instead tested by executing it, and reviewed by two bots across five rounds — 13 findings, all verified against the repo before acting, all resolved:web-exportbefore reporting a missingNATIVE_ONLY_MODULESentry is backwards — a dep imported only from.native.files keeps the export green and still must be registered. My own text said so two paragraphs earlier, so the correction contradicted the thing it was correcting.Writefromallowed-toolswould break the Output step, since the skill's deliverable is a written report. The contract is "changes nothing in the repo", which is whyEditis absent and the report goes to the scratchpad. That decline was accepted.Known gaps, stated plainly:
check:paritynever ran, so nothing server-side was verified in the run. The pull-path checks were vacuous rather than clean —conflict.ts,cursors.tsandpaginate.tshave no callers yet, so that half of the sync contract has never been audited and will need a pass of its own when M3b lands.