-
Notifications
You must be signed in to change notification settings - Fork 1
feat(review): add lane-based Codex review loop #68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
e2ba25f
feat(review): add lane-based Codex review loop
choi138 995c822
fix(review): address review-loop findings
choi138 d395584
fix(review): harden scope safety and finding counts
choi138 150d3aa
fix(review): close follow-up safety gaps
choi138 23f6817
fix(review): harden scope and path handling
choi138 83e133d
fix(review): suppress filters and malformed strings
choi138 457973a
fix(review): enforce scope invariants and document trust boundary
choi138 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,157 @@ | ||
| --- | ||
| name: codex-review-loop | ||
| description: Review Toki uncommitted changes, branches, or commits with path- and semantic-routed specialist lanes, normalize P0-P3 findings, and optionally fix only findings the user explicitly approves. Use when the user asks for a local code review, review loop, review-and-fix pass, re-review, or approved finding remediation in the Toki repository. Keep GitHub review submission, PR comments, push, merge, and other remote writes outside this skill. | ||
| --- | ||
|
|
||
| # Toki Codex Review Loop | ||
|
|
||
| Run an rvw-compatible lane review, then apply the codex-lb-style approval, | ||
| atomic-fix, verification, and bounded re-review loop. | ||
|
|
||
| ## Core Contract | ||
|
|
||
| - Treat every review request as read-only until the user explicitly approves a | ||
| write mode. | ||
| - Never infer permission to edit or commit from phrases such as "review this" | ||
| or from a GitHub `@codex review` request. | ||
| - Never push, submit a GitHub review, comment, resolve threads, label, merge, or | ||
| otherwise mutate remote state from this skill. | ||
| - Preserve unrelated staged, unstaged, and untracked changes. Never use | ||
| `git reset`, `git checkout`, or broad restoration to undo a finding fix. | ||
| - Treat local usage logs, databases, audit findings, prompts, transcripts, and | ||
| credentials as sensitive. Do not print raw matched diff content. | ||
| - Apply `project-conventions` and read its task-specific references before | ||
| fixing Toki source, tests, resources, or project configuration. | ||
| - Apply `.agents/conventions/git-workflow.md` before any commit. A commit | ||
| requires explicit user authorization separate from review authorization. | ||
|
|
||
| ## Workflow | ||
|
|
||
| ### 1. Resolve One Review Scope | ||
|
|
||
| Choose exactly one scope: | ||
|
|
||
| - Use `--uncommitted` for staged, unstaged, and untracked changes. | ||
| - Use `--base <branch>` for the current branch relative to a base branch. | ||
| - Use `--commit <sha>` for one commit. | ||
|
|
||
| Prefer an exact scope named by the user. Do not fetch or change branches merely | ||
| to infer a scope. | ||
|
|
||
| Run the resolver: | ||
|
|
||
| ~~~bash | ||
| python3 .agents/skills/codex-review-loop/scripts/resolve_review_scope.py \ | ||
| --repo . \ | ||
| --uncommitted \ | ||
| --pretty | ||
| ~~~ | ||
|
|
||
| Stop before invoking Codex when `safeToReview` is false or `hasChanges` is | ||
| false. Excluded sensitive paths make the scope unsafe because | ||
| `codex exec review --uncommitted` cannot exclude individual paths. | ||
|
|
||
| ### 2. Load Only Activated Review Rules | ||
|
|
||
| Always read the common reviewer prompt and baseline lane. Read only the | ||
| specialist lane files named by `activatedLanes`. Also read the finding schema | ||
| and verification reference before reporting or fixing findings. | ||
|
|
||
| Treat lane activation as additive. Never disable baseline correctness or | ||
| security reasoning merely because a path pattern did not match. | ||
|
|
||
| ### 3. Run One Pass Per Activated Lane | ||
|
|
||
| Run the baseline lane and every activated specialist lane: | ||
|
|
||
| ~~~bash | ||
| .agents/skills/codex-review-loop/scripts/run_review_lane.sh \ | ||
| --repo . \ | ||
| --lane baseline \ | ||
| --base main | ||
| ~~~ | ||
|
|
||
| The runner sends the custom prompt through stdin, uses the structured-output | ||
| schema, requests an ephemeral Codex session, and refuses inactive or unsafe | ||
| lanes. Run lanes sequentially by default. Do not add replicated reviewers or | ||
| an adjudicator unless the user explicitly expands the workflow. | ||
|
|
||
| ### 4. Validate And Merge Findings | ||
|
|
||
| Capture each lane's JSON result in a temporary directory, then merge: | ||
|
|
||
| ~~~bash | ||
| python3 .agents/skills/codex-review-loop/scripts/merge_findings.py merge \ | ||
| /tmp/toki-review/baseline.json \ | ||
| /tmp/toki-review/remote-sync.json | ||
| ~~~ | ||
|
|
||
| Reject malformed results. Conservatively merge overlapping findings with a | ||
| similar root cause, retain every contributing lane, use the highest priority, | ||
| and surface materially conflicting priority opinions. | ||
|
|
||
| ### 5. Report Before Writing | ||
|
|
||
| Present the normalized findings with ID, priority, confidence, location, impact, | ||
| suggested fix, and verification plan. Then request one explicit mode: | ||
|
|
||
| 1. Report only. | ||
| 2. Fix selected findings without commits. | ||
| 3. Fix all actionable findings without commits. | ||
| 4. Fix selected findings and create one verified commit per finding. | ||
| 5. Fix all actionable findings and create one verified commit per finding. | ||
|
|
||
| Treat "Fix P0/P1 only" as a valid selection. For a P0/P1 fix that changes | ||
| existing product behavior rather than restoring clearly intended behavior, | ||
| describe that behavior change and obtain a second confirmation. | ||
|
|
||
| ### 6. Fix One Finding At A Time | ||
|
|
||
| Before each fix: | ||
|
|
||
| 1. Re-read the affected code and relevant `project-conventions` references. | ||
| 2. Inspect `git status` and the affected diff. | ||
| 3. Record the pre-fix state of only the files or hunks that will change. | ||
| 4. Use `apply_patch` for the narrow fix. | ||
| 5. Run the smallest verification profile that can prove the finding fixed. | ||
|
|
||
| If verification fails, reverse only the patch introduced for that finding. If | ||
| that boundary cannot be proven, stop and report the failure instead of risking | ||
| the user's work. | ||
|
|
||
| For `--uncommitted` reviews, default to no commits. Create atomic commits only | ||
| when the user explicitly selects a commit mode and the new fix can be separated | ||
| from pre-existing changes without staging unrelated hunks. | ||
|
|
||
| ### 7. Re-review With A Bound | ||
|
|
||
| After approved fixes, rerun baseline plus only lanes affected by the fix. Stop | ||
| after three total review rounds. If the same root cause returns in two | ||
| consecutive rounds, mark it `wont_fix` with the reason instead of looping. | ||
|
|
||
| ### 8. Finish With A Local Report | ||
|
|
||
| Report: | ||
|
|
||
| - Review scope and activated lanes. | ||
| - Findings fixed, skipped, `wont_fix`, or left for reporting only. | ||
| - Verification commands and outcomes. | ||
| - Commits created, if explicitly authorized. | ||
| - Remaining risks or unverified checks. | ||
|
|
||
| State explicitly that no push or GitHub write occurred. | ||
|
|
||
| ## Reference Map | ||
|
|
||
| - Common contract: [reviewer.md](references/prompts/reviewer.md) | ||
| - Lane registry: [lane-registry.json](references/lane-registry.json) | ||
| - Finding schema: [review-findings.schema.json](references/schemas/review-findings.schema.json) | ||
| - Verification profiles: [verification.md](references/verification.md) | ||
| - Baseline: [baseline.md](references/lanes/baseline.md) | ||
| - Usage and pricing: [usage-pricing.md](references/lanes/usage-pricing.md) | ||
| - Privacy and security: [privacy-security.md](references/lanes/privacy-security.md) | ||
| - Remote sync: [remote-sync.md](references/lanes/remote-sync.md) | ||
| - Concurrency and lifecycle: [concurrency-lifecycle.md](references/lanes/concurrency-lifecycle.md) | ||
| - SwiftUI architecture: [swiftui-architecture.md](references/lanes/swiftui-architecture.md) | ||
| - Build and portability: [build-portability.md](references/lanes/build-portability.md) | ||
| - Testing: [testing.md](references/lanes/testing.md) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| interface: | ||
| display_name: "Toki Codex Review Loop" | ||
| short_description: "Route Toki review lanes and fix approved findings" | ||
| default_prompt: "Use $codex-review-loop to review the current Toki changes and report findings before any fixes." |
168 changes: 168 additions & 0 deletions
168
.agents/skills/codex-review-loop/references/lane-registry.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,168 @@ | ||
| { | ||
| "version": "1.0", | ||
| "pathExclusions": [ | ||
| ".git", | ||
| ".git/**", | ||
| ".hermes", | ||
| ".hermes/**", | ||
| ".omo", | ||
| ".omo/**", | ||
| ".senpi", | ||
| ".senpi/**", | ||
| ".codegraph", | ||
| ".codegraph/**", | ||
| "build", | ||
| "build/**", | ||
| "DerivedData", | ||
| "DerivedData/**" | ||
| ], | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| "lanes": [ | ||
| { | ||
| "id": "baseline", | ||
| "always": true, | ||
| "prompt": "references/lanes/baseline.md", | ||
| "pathPatterns": [], | ||
| "semanticPatterns": [], | ||
| "verificationProfiles": ["common"], | ||
| "execution": {"replicas": 1, "adjudication": false} | ||
| }, | ||
| { | ||
| "id": "usage-pricing", | ||
| "always": false, | ||
| "prompt": "references/lanes/usage-pricing.md", | ||
| "pathPatterns": [ | ||
| "Toki/Domain/Usage/**", | ||
| "Toki/Infrastructure/UsageReaders/**", | ||
| "Sources/TokiUsageCore/**", | ||
| "Sources/TokiUsageReaders/**", | ||
| "TokiTests/*Usage*", | ||
| "TokiTests/*Pricing*", | ||
| "TokiTests/*Reader*", | ||
| "Tests/TokiAgentTests/*Usage*", | ||
| "Tests/TokiAgentTests/*Ledger*" | ||
| ], | ||
| "semanticPatterns": [ | ||
| "(?i)\\b(token|usage|cost|price|pricing|model|attribution|timezone|date\\s*range|active\\s*time)\\b" | ||
| ], | ||
| "verificationProfiles": ["swift-package", "app-tests"], | ||
| "execution": {"replicas": 1, "adjudication": false} | ||
| }, | ||
| { | ||
| "id": "privacy-security", | ||
| "always": false, | ||
| "prompt": "references/lanes/privacy-security.md", | ||
| "pathPatterns": [ | ||
| "Toki/Domain/SecurityAudit/**", | ||
| "Toki/Infrastructure/SecurityAudit/**", | ||
| "Sources/TokiDurableStorage/**", | ||
| "Sources/TokiUsageReaders/**", | ||
| "TokiTests/*Security*", | ||
| "Tests/**/*Security*", | ||
| "Tests/**/*Privacy*" | ||
| ], | ||
| "semanticPatterns": [ | ||
| "(?i)\\b(secret|credential|api[_ -]?key|jwt|private[_ -]?key|mask|redact|sqlite|database|transcript|prompt|telemetry)\\b" | ||
| ], | ||
| "verificationProfiles": ["swift-package", "app-tests"], | ||
| "execution": {"replicas": 1, "adjudication": false} | ||
| }, | ||
| { | ||
| "id": "remote-sync", | ||
| "always": false, | ||
| "prompt": "references/lanes/remote-sync.md", | ||
| "pathPatterns": [ | ||
| "Toki/Infrastructure/RemoteSync/**", | ||
| "Sources/TokiSyncProtocol/**", | ||
| "Sources/TokiAgentCore/**", | ||
| "Sources/TokiAgent/**", | ||
| "Tests/TokiSyncProtocolTests/**", | ||
| "Tests/TokiAgentTests/**", | ||
| "TokiHub/Sources/**", | ||
| "TokiHub/Tests/**" | ||
| ], | ||
| "semanticPatterns": [ | ||
| "(?i)\\b(nonce|cipher|encrypt|decrypt|signature|replay|rollback|snapshot|anchor|pairing|durable|hub\\s*route)\\b" | ||
| ], | ||
| "verificationProfiles": ["swift-package", "hub"], | ||
| "execution": {"replicas": 1, "adjudication": false} | ||
| }, | ||
| { | ||
| "id": "concurrency-lifecycle", | ||
| "always": false, | ||
| "prompt": "references/lanes/concurrency-lifecycle.md", | ||
| "pathPatterns": [ | ||
| "Toki/App/**", | ||
| "Toki/Features/**", | ||
| "Toki/Infrastructure/Activity/**", | ||
| "Toki/Infrastructure/RemoteSync/**", | ||
| "Sources/TokiAgentCore/**", | ||
| "TokiHub/Sources/**" | ||
| ], | ||
| "semanticPatterns": [ | ||
| "(?i)\\b(Task|actor|MainActor|Sendable|async|await|Timer|cancell?ation|lock|mutex)\\b" | ||
| ], | ||
| "verificationProfiles": ["swift-package", "hub", "app-tests"], | ||
| "execution": {"replicas": 1, "adjudication": false} | ||
| }, | ||
| { | ||
| "id": "swiftui-architecture", | ||
| "always": false, | ||
| "prompt": "references/lanes/swiftui-architecture.md", | ||
| "pathPatterns": [ | ||
| "Toki/App/**", | ||
| "Toki/Features/**", | ||
| "TokiTests/*Panel*", | ||
| "TokiTests/*ViewModel*", | ||
| "TokiTests/*MenuBar*" | ||
| ], | ||
| "semanticPatterns": [ | ||
| "(?i)(SwiftUI|@State\\b|@StateObject\\b|@ObservedObject\\b|@Environment\\b|ViewModel\\b|NSStatusItem\\b|NSPanel\\b)" | ||
| ], | ||
| "verificationProfiles": ["app-format", "app-lint", "app-tests"], | ||
| "execution": {"replicas": 1, "adjudication": false} | ||
| }, | ||
| { | ||
| "id": "build-portability", | ||
| "always": false, | ||
| "prompt": "references/lanes/build-portability.md", | ||
| "pathPatterns": [ | ||
| "Package.swift", | ||
| "Package.resolved", | ||
| "TokiHub/Package.swift", | ||
| "TokiHub/Package.resolved", | ||
| "project.yml", | ||
| ".swiftformat", | ||
| ".swiftlint.yml", | ||
| ".github/workflows/**", | ||
| "Toki.xcodeproj/**", | ||
| "Sources/CSQLite/**", | ||
| "Toki/Resources/**", | ||
| "Toki/Assets.xcassets/**" | ||
| ], | ||
| "semanticPatterns": [ | ||
| "(?i)\\b(PackageDescription|swift-tools-version|dependency|xcodegen|resource|Vapor|Linux|CSQLite|Package\\.resolved)\\b" | ||
| ], | ||
| "verificationProfiles": ["swift-package", "hub", "app-format", "app-lint", "project"], | ||
| "execution": {"replicas": 1, "adjudication": false} | ||
| }, | ||
| { | ||
| "id": "testing", | ||
| "always": false, | ||
| "prompt": "references/lanes/testing.md", | ||
| "pathPatterns": [ | ||
| "Toki/**/*.swift", | ||
| "TokiTests/*.swift", | ||
| "TokiTests/**/*.swift", | ||
| "Sources/**/*.swift", | ||
| "Tests/**/*.swift", | ||
| "TokiHub/Sources/**/*.swift", | ||
| "TokiHub/Tests/**/*.swift" | ||
| ], | ||
| "semanticPatterns": [ | ||
| "(?i)\\b(XCTest|Testing|test[A-Z_]|assert|fixture|mock|stub)\\b" | ||
| ], | ||
| "verificationProfiles": ["swift-package", "hub", "app-tests"], | ||
| "execution": {"replicas": 1, "adjudication": false} | ||
| } | ||
| ] | ||
| } | ||
15 changes: 15 additions & 0 deletions
15
.agents/skills/codex-review-loop/references/lanes/baseline.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| # Baseline Lane | ||
|
|
||
| Review every change for: | ||
|
|
||
| - Incorrect behavior, broken invariants, off-by-one and boundary mistakes. | ||
| - Missing error handling, failure propagation, or invalid fallback behavior. | ||
| - Crashes, data loss, corruption, stale state, and silent partial success. | ||
| - Compatibility regressions in public or persisted data contracts. | ||
| - Unsafe assumptions about optional values, file presence, ordering, or input | ||
| shape. | ||
| - Material performance regressions on frequently executed paths. | ||
|
|
||
| Trace important callers and consumers when the changed code alters a contract. | ||
| Do not duplicate a specialist finding unless baseline reasoning independently | ||
| identifies the same root cause. |
16 changes: 16 additions & 0 deletions
16
.agents/skills/codex-review-loop/references/lanes/build-portability.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| # Build And Portability Lane | ||
|
|
||
| Review build, package, resource, and CI changes for: | ||
|
|
||
| - Swift 5.9.2 compatibility for the Linux Agent and Hub. | ||
| - macOS-only imports or APIs leaking into cross-platform package targets. | ||
| - CSQLite availability, conditional dependencies, linker assumptions, and | ||
| platform guards. | ||
| - Vapor dependencies remaining isolated to the Hub package. | ||
| - XcodeGen source/resource membership, build settings, generated project drift, | ||
| and case-sensitive paths. | ||
| - Package resolution or lockfile changes that are missing, unintended, or | ||
| inconsistent across root, Hub, and Xcode workspaces. | ||
| - CI jobs no longer exercising required builds or tests. | ||
|
|
||
| Prefer `project.yml` plus regeneration over direct project-file edits. |
19 changes: 19 additions & 0 deletions
19
.agents/skills/codex-review-loop/references/lanes/concurrency-lifecycle.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| # Concurrency And Lifecycle Lane | ||
|
|
||
| Review asynchronous and lifecycle behavior for: | ||
|
|
||
| - Actor isolation, `Sendable` assumptions, shared mutable state, and race | ||
| conditions. | ||
| - Main-actor blocking by file IO, SQLite, network calls, parsing, scanning, or | ||
| aggregation. | ||
| - Unstructured tasks that outlive their owner, duplicate work, or ignore | ||
| cancellation. | ||
| - Timer and notification lifecycles, retain cycles, repeated registration, and | ||
| missed teardown. | ||
| - Lock ordering, reentrancy, deadlocks, and state read outside protection. | ||
| - Stale async results overwriting newer refresh or configuration state. | ||
| - Errors or cancellation converted into successful or permanently loading UI | ||
| state. | ||
|
|
||
| Verify ownership from creation through cancellation and deinitialization, not | ||
| only the task body. |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.