feat(review): add lane-based Codex review loop - #68
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCodex 기반 리뷰 루프를 추가했습니다. Git 범위를 해석하고 specialist lane을 선택합니다. Codex 결과를 검증하고 finding을 병합합니다. 승인된 수정과 최대 3회의 재검토 절차를 정의했습니다. ChangesCodex 리뷰 루프
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant run_review_lane.sh
participant resolve_review_scope.py
participant Codex
participant merge_findings.py
Developer->>run_review_lane.sh: 리뷰 범위와 lane 지정
run_review_lane.sh->>resolve_review_scope.py: 범위 해석 요청
resolve_review_scope.py-->>run_review_lane.sh: 활성 lane과 검증 프로파일 반환
run_review_lane.sh->>Codex: reviewer prompt와 변경 범위 전달
Codex-->>run_review_lane.sh: JSON finding 결과 작성
run_review_lane.sh->>merge_findings.py: 결과 schema 검증
merge_findings.py-->>Developer: 병합된 리뷰 결과 반환
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![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: e2ba25fe8b
ℹ️ 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".
choi138
added
the
🤖 codex: needs work
![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)
.agents/skills/codex-review-loop/references/schemas/review-findings.schema.json (1)
17-86: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win스키마 제약을
merge_findings.py의 검증 규칙과 맞추십시오.
merge_findings.py의validate_lane_result는 lane result의 필드 집합,schemaVersion,lane형식,verdict,findings배열, priority, confidence 범위(0-1), title/rootCause/evidence/impact/suggestedFix의 비어 있지 않음, 저장소 상대 경로, startLine/endLine, verification 배열의 비어 있지 않은 문자열을 강제합니다. 이 스키마 파일에는 이러한 제약 중 다수(confidence 범위, 문자열 최소 길이, verification 최소 항목 수, startLine 최소값, lane 패턴)가 없습니다.Codex는
--output-schema로 이 스키마만 보고 출력을 생성합니다. 스키마가 느슨하면 스키마상 유효하지만merge_findings.py에서 거부되는 출력을 생성할 수 있고, 이는 Codex 호출 낭비와 재실행으로 이어집니다.♻️ 제안된 스키마 강화
"lane": { - "type": "string" + "type": "string", + "pattern": "^[a-z0-9]+(-[a-z0-9]+)*$" }, "verdict": { "type": "string", "enum": ["clean", "findings"] }, "summary": { - "type": "string" + "type": "string", + "minLength": 1 }, "findings": { "type": "array", "items": { "type": "object", "additionalProperties": false, ... "properties": { "priority": { "type": "string", "enum": ["P0", "P1", "P2", "P3"] }, "confidence": { - "type": "number" + "type": "number", + "minimum": 0, + "maximum": 1 }, "title": { - "type": "string" + "type": "string", + "minLength": 1 }, "file": { "type": "string" }, "startLine": { - "type": "integer" + "type": "integer", + "minimum": 1 }, "endLine": { - "type": "integer" + "type": "integer", + "minimum": 1 }, "rootCause": { - "type": "string" + "type": "string", + "minLength": 1 }, "evidence": { - "type": "string" + "type": "string", + "minLength": 1 }, "impact": { - "type": "string" + "type": "string", + "minLength": 1 }, "suggestedFix": { - "type": "string" + "type": "string", + "minLength": 1 }, "verification": { "type": "array", + "minItems": 1, "items": { - "type": "string" + "type": "string", + "minLength": 1 } } } } }🤖 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 @.agents/skills/codex-review-loop/references/schemas/review-findings.schema.json around lines 17 - 86, Align the review-findings JSON schema with merge_findings.py’s validate_lane_result rules: add the required schemaVersion and lane constraints, enforce the exact allowed field set, validate verdict and findings as required, constrain priority and confidence to their accepted values/range, require non-empty title/rootCause/evidence/impact/suggestedFix, enforce repository-relative file paths and startLine/endLine minimums, and require verification to contain non-empty strings with the validator’s minimum count. Update the corresponding required/properties definitions so Codex output accepted by the schema is also accepted by validate_lane_result.
🤖 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 @.agents/skills/codex-review-loop/references/lane-registry.json:
- Around line 3-18: 보안 파일이 리뷰 범위에 포함되지 않도록 `pathExclusions`에 `.env`, `*.pem`,
`*.key`, `*.p12`, `id_rsa*` 패턴과 필요한 변형을 추가하세요. 기존 디렉터리 제외 항목은 유지하고,
`safeToReview: true` 경로에서도 일반적인 자격증명 파일이 fail-closed로 제외되도록 구성하세요.
In @.agents/skills/codex-review-loop/scripts/merge_findings.py:
- Line 32: Update WORD_RE and the normalized_words flow used by finding_id and
similarity to tokenize Unicode letters and numbers, not only ASCII characters.
Preserve the existing normalization, hashing, and similarity behavior while
ensuring non-ASCII-only titles or rootCause values produce meaningful tokens,
distinct finding IDs, and token-based similarity.
In @.agents/skills/codex-review-loop/scripts/run_review_lane.sh:
- Around line 142-156: Update the codex review invocation in the
TOKI_REVIEW_CHILD subshell to avoid passing scope arguments from
codex_scope_args together with the stdin prompt marker "-". Preserve the
selected review scope by invoking codex with the scope flags only, and remove
the conflicting prompt input path for this review-mode call.
---
Nitpick comments:
In
@.agents/skills/codex-review-loop/references/schemas/review-findings.schema.json:
- Around line 17-86: Align the review-findings JSON schema with
merge_findings.py’s validate_lane_result rules: add the required schemaVersion
and lane constraints, enforce the exact allowed field set, validate verdict and
findings as required, constrain priority and confidence to their accepted
values/range, require non-empty title/rootCause/evidence/impact/suggestedFix,
enforce repository-relative file paths and startLine/endLine minimums, and
require verification to contain non-empty strings with the validator’s minimum
count. Update the corresponding required/properties definitions so Codex output
accepted by the schema is also accepted by validate_lane_result.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d5d8f32-c22d-4567-a7d3-033082f55cda
📒 Files selected for processing (25)
.agents/hooks/skill_activation.ts.agents/skills/codex-review-loop/SKILL.md.agents/skills/codex-review-loop/agents/openai.yaml.agents/skills/codex-review-loop/references/lane-registry.json.agents/skills/codex-review-loop/references/lanes/baseline.md.agents/skills/codex-review-loop/references/lanes/build-portability.md.agents/skills/codex-review-loop/references/lanes/concurrency-lifecycle.md.agents/skills/codex-review-loop/references/lanes/privacy-security.md.agents/skills/codex-review-loop/references/lanes/remote-sync.md.agents/skills/codex-review-loop/references/lanes/swiftui-architecture.md.agents/skills/codex-review-loop/references/lanes/testing.md.agents/skills/codex-review-loop/references/lanes/usage-pricing.md.agents/skills/codex-review-loop/references/prompts/reviewer.md.agents/skills/codex-review-loop/references/schemas/review-findings.schema.json.agents/skills/codex-review-loop/references/verification.md.agents/skills/codex-review-loop/scripts/merge_findings.py.agents/skills/codex-review-loop/scripts/resolve_review_scope.py.agents/skills/codex-review-loop/scripts/run_review_lane.sh.agents/skills/codex-review-loop/tests/test_activation_hook.py.agents/skills/codex-review-loop/tests/test_finding_merge.py.agents/skills/codex-review-loop/tests/test_runner_contract.py.agents/skills/codex-review-loop/tests/test_scope_routing.py.agents/skills/skill-rules.json.gitignoreAGENTS.md
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
21-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNode 22 호환성을 고려해
--experimental-strip-types사용을 제거하거나 CI Node 버전을 고정하십시오.Node 22.18 이상에서는 타입 스트립이 기본적으로 활성화됩니다.
.github/workflows/ci.yml의node-version: 22가 최신 패치를 설치할 수 있어 테스트 의도와 실행 환경이 달라질 수 있습니다.subprocess.run(..., check=True)경로에서 Node 플래그 변경이 CI 실패로 이어지지 않도록 테스트를 최신 Node 22 동작에 맞추고, 특정 패치에서 CI를 실행하려면node-version을 고정하십시오.🤖 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 @.github/workflows/ci.yml around lines 21 - 27, Pin the Node.js version in the “Set up Node.js for review-loop hook tests” step to an exact Node 22 patch version compatible with the existing test commands, rather than the floating `22` alias. Keep the `Test Codex review loop` command unchanged and ensure CI consistently uses the intended Node behavior.
🤖 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.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 21-27: Pin the Node.js version in the “Set up Node.js for
review-loop hook tests” step to an exact Node 22 patch version compatible with
the existing test commands, rather than the floating `22` alias. Keep the `Test
Codex review loop` command unchanged and ensure CI consistently uses the
intended Node behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 455f28af-b90a-446c-a625-83f70dbe9767
📒 Files selected for processing (15)
.agents/skills/codex-review-loop/references/lane-registry.json.agents/skills/codex-review-loop/references/prompts/reviewer.md.agents/skills/codex-review-loop/scripts/finding_validation.py.agents/skills/codex-review-loop/scripts/merge_findings.py.agents/skills/codex-review-loop/scripts/resolve_review_scope.py.agents/skills/codex-review-loop/scripts/review_git_process.py.agents/skills/codex-review-loop/scripts/review_scope_git.py.agents/skills/codex-review-loop/scripts/run_review_lane.sh.agents/skills/codex-review-loop/tests/test_activation_hook.py.agents/skills/codex-review-loop/tests/test_finding_merge.py.agents/skills/codex-review-loop/tests/test_runner_contract.py.agents/skills/codex-review-loop/tests/test_scope_routing.py.agents/skills/codex-review-loop/tests/test_scope_security.py.agents/skills/skill-rules.json.github/workflows/ci.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- .agents/skills/skill-rules.json
- .agents/skills/codex-review-loop/references/lane-registry.json
- .agents/skills/codex-review-loop/references/prompts/reviewer.md
- .agents/skills/codex-review-loop/scripts/run_review_lane.sh
choi138
removed
the
🤖 codex: needs work
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 995c822318
ℹ️ 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".
choi138
added
the
🤖 codex: needs work
choi138
removed
the
🤖 codex: needs work
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d395584fc7
ℹ️ 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".
choi138
added
the
🤖 codex: needs work
choi138
removed
the
🤖 codex: needs work
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 150d3aa385
ℹ️ 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".
choi138
added
the
🤖 codex: needs work
choi138
removed
the
🤖 codex: needs work
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23f681744f
ℹ️ 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".
choi138
added
the
🤖 codex: needs work
choi138
removed
the
🤖 codex: needs work
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.agents/skills/codex-review-loop/scripts/review_git_process.py (1)
35-73: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
filter.process무력화 시나리오에 대한 전용 테스트가 없습니다.
git_environment_without_filters는clean,smudge,process,required중 하나라도 설정된 드라이버를 발견하면 네 가지 항목을 모두 재정의합니다.GIT_CONFIG_KEY/GIT_CONFIG_VALUE환경변수 오버라이드는 설정 파일의 값보다 우선하므로, 원본 악성 명령은 어떤 경우에도 실행되지 않습니다. 이 설계 자체는 타당합니다.다만
test_scope_security.py와test_runner_contract.py의 clean filter 테스트는filter.<driver>.clean만 설정합니다.filter.<driver>.process가 설정된 드라이버는 clean/smudge보다 항상 우선 적용되는데(long-running process filter가 우선), 이 경로를 검증하는 테스트가 없습니다.process오버라이드 값이 빈 문자열일 때 Git이 이를 실제로 무력화(no-op)로 처리하는지, 혹은 실행 실패로 처리되어required=false로 인해 원본 콘텐츠로 안전하게 대체되는지 확인하는 회귀 테스트를 추가하는 것이 좋습니다.Git이 빈
filter.<driver>.process값을 어떻게 처리하는지 최신 문서로 확인해 주시겠습니까?🤖 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 @.agents/skills/codex-review-loop/scripts/review_git_process.py around lines 35 - 73, Verify from current Git documentation how an empty filter.<driver>.process override is handled, then add dedicated regression coverage in the existing test_scope_security.py and test_runner_contract.py filter tests. Configure a driver with process (and required as applicable), exercise the relevant Git operation, and assert the malicious process is never executed while the result follows Git’s documented no-op or safe fallback behavior.
🤖 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.
Nitpick comments:
In @.agents/skills/codex-review-loop/scripts/review_git_process.py:
- Around line 35-73: Verify from current Git documentation how an empty
filter.<driver>.process override is handled, then add dedicated regression
coverage in the existing test_scope_security.py and test_runner_contract.py
filter tests. Configure a driver with process (and required as applicable),
exercise the relevant Git operation, and assert the malicious process is never
executed while the result follows Git’s documented no-op or safe fallback
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f4ca8a5f-65b9-44aa-b7e0-0fd5fe16d625
📒 Files selected for processing (9)
.agents/skills/codex-review-loop/references/lane-registry.json.agents/skills/codex-review-loop/scripts/finding_validation.py.agents/skills/codex-review-loop/scripts/review_git_process.py.agents/skills/codex-review-loop/scripts/review_scope_git.py.agents/skills/codex-review-loop/scripts/run_review_lane.sh.agents/skills/codex-review-loop/scripts/run_with_safe_git.py.agents/skills/codex-review-loop/tests/test_finding_merge.py.agents/skills/codex-review-loop/tests/test_runner_contract.py.agents/skills/codex-review-loop/tests/test_scope_security.py
🚧 Files skipped from review as they are similar to previous changes (3)
- .agents/skills/codex-review-loop/scripts/run_review_lane.sh
- .agents/skills/codex-review-loop/references/lane-registry.json
- .agents/skills/codex-review-loop/scripts/review_scope_git.py
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83e133d1ad
ℹ️ 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".
| def git_environment_without_filters(repo: Path) -> dict[str, str]: | ||
| environment = dict(os.environ) | ||
| result = subprocess.run( |
There was a problem hiding this comment.
When Git configures core.fsmonitor as an external hook, the resolver's worktree git diff commands execute that program before safeToReview is known; I reproduced this with a marker-writing hook and resolve_review_scope.py --uncommitted. The environment built here disables content filters but not fsmonitor, and it is also inherited by native Codex, so initiating a review can run arbitrary local tooling that mutates state or transmits sensitive data. Git documents that a pathname value for core.fsmonitor is treated as a hook command; override it to false for every resolver and child command and add a marker-hook regression test.
AGENTS.md reference: AGENTS.md:L33-L35
Useful? React with 👍 / 👎.
| def base_scope(repo: Path, base: str) -> tuple[list[str], bytes, bool]: | ||
| validate_base(repo, base) |
There was a problem hiding this comment.
When --base or --commit runs from a checkout containing an ignored sensitive file such as .env, these scope helpers inspect only paths in the selected Git diff and report safeToReview: true; native Codex can still open that worktree file while inspecting surrounding code. Fresh evidence beyond the earlier --uncommitted fix is that ignored_paths_matching is called only from uncommitted_scope; I reproduced both base and commit scopes returning no exclusions with an ignored .env present. Apply the ignored-exclusion scan to every scope or run reviews in a sanitized checkout, with regressions for both flags.
AGENTS.md reference: AGENTS.md:L33-L35
Useful? React with 👍 / 👎.
| untracked_roots = decode_z( | ||
| run_git(repo, ["ls-files", "--others", "--exclude-standard", "--directory", "-z"]) | ||
| ) |
There was a problem hiding this comment.
When an external symlink has an ordinary ignored name, --exclude-standard removes it from untracked, while ignored_paths_matching inventories only names matching sensitive-path patterns; the later symlink guard therefore never sees it and native --uncommitted review can follow its target. Fresh evidence beyond the previously fixed non-ignored symlink case is that I reproduced safeToReview: true with a tracked change plus an ignored link pointing to an external sensitive file. Inventory ignored symlinks independently of filename exclusions, fail closed before Codex invocation, and add this case to the symlink regressions.
AGENTS.md reference: AGENTS.md:L33-L35
Useful? React with 👍 / 👎.
| parts = path.split("/") | ||
| if any(part in {"", ".", ".."} for part in parts): | ||
| raise FindingError( |
There was a problem hiding this comment.
When a lane emits a valid JSON escape such as "bad\u0000name.swift", this path passes both strict UTF-8 encoding and the segment checks, and the merger accepts it unchanged; I reproduced both validation and merge succeeding. Any later filesystem or Git operation used to remediate the selected finding rejects the embedded NUL, so the generated RF-* entry cannot identify or fix a repository file. Reject NUL bytes in validate_repo_path and add validation-and-merge coverage for this input.
Useful? React with 👍 / 👎.
| if "baseline" not in seen_ids: | ||
| raise ScopeError("lane registry must include baseline") |
There was a problem hiding this comment.
When a caller supplies the supported --registry option with a baseline entry whose always field is false or absent, this validation accepts it, but a change matching no baseline path or semantic patterns activates no baseline review; run_review_lane.sh --lane baseline then refuses to run. This defeats the review loop's invariant that baseline correctness and security reasoning always apply. Require the baseline entry's always value to be the literal boolean true and add malformed-registry coverage.
AGENTS.md reference: AGENTS.md:L12-L12
Useful? React with 👍 / 👎.
| for finding in result["findings"]: | ||
| duplicate = next((group for group in groups if is_duplicate(group, finding)), None) | ||
| if duplicate is None: | ||
| groups.append(new_group(lane, finding)) | ||
| else: | ||
| merge_into(duplicate, lane, finding) |
There was a problem hiding this comment.
When two same-cause groups are initially non-overlapping and a later finding overlaps both, this next expression merges the bridge into only the first group; its expanded range can then overlap the second group without any consolidation pass. I reproduced results containing (1–5) and (5–6) groups with identical root causes after findings (1–2), (5–6), and (2–5) arrived in that order. The report consequently assigns two IDs to one defect and splits occurrences and evidence, so selected remediation can target the same issue twice. Merge all matching groups and consolidate them transitively, with order-permutation coverage for the bridge case.
AGENTS.md reference: AGENTS.md:L12-L12
Useful? React with 👍 / 👎.
choi138
added
the
🤖 codex: needs work
Close the six open Codex findings at the invariant level rather than at each reported case, and record the boundary those invariants defend. - Resolve git and the review binary from the platform default path, and narrow PATH before any external command runs. - Override repository-configured content filters, textconv, external diff, core.fsmonitor, core.hooksPath, and lazy fetch, or refuse the scope. - Derive workspace confinement from a filesystem walk that validates every symbolic link target, so ignore rules and index modes cannot hide a path. - Apply the excluded-path scan to base and commit scopes, not only uncommitted. - Pass the repository root as NUL-terminated data to preserve exact names. - Reject NUL in finding paths, and require the baseline lane to stay always-on. - Group findings with order-independent connected components so a bridging finding coalesces every matching group. Document the untrusted inputs, enforced invariants, and non-goals in SKILL.md, and count externally triggered rounds against the re-review bound.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.agents/skills/codex-review-loop/tests/test_runner_contract.py (1)
105-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
use_default_codex=True일 때 상속된TOKI_REVIEW_CODEX_BIN을 제거하십시오.
environment는os.environ.copy()로 시작합니다. 상위 환경에TOKI_REVIEW_CODEX_BIN이 설정되어 있으면use_default_codex=True에서도 그 값이 상속됩니다. 그 값이 존재하지 않는 경로를 가리키면 러너는 PATH 해석 로직에 도달하기 전에 exit 2를 반환합니다.test_rejects_repository_local_codex_from_path는 그 경우에도 통과하지만 검증 대상 로직을 실제로 시험하지 않습니다.♻️ 제안된 수정
environment = os.environ.copy() if not use_default_codex: environment["TOKI_REVIEW_CODEX_BIN"] = str(self.fake_codex) + else: + environment.pop("TOKI_REVIEW_CODEX_BIN", None) environment["TOKI_REVIEW_CAPTURE_FILE"] = str(self.capture)🤖 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 @.agents/skills/codex-review-loop/tests/test_runner_contract.py around lines 105 - 117, Update run_runner so use_default_codex=True removes any inherited TOKI_REVIEW_CODEX_BIN from the copied environment before invoking the runner; retain the fake-codex override when use_default_codex is false and preserve explicit environment_overrides handling..agents/skills/codex-review-loop/scripts/review_git_process.py (1)
144-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
resolved할당과relative_to검사를 분리하십시오.현재
try블록은Path(entry).resolve()와resolved.relative_to(...)를 함께 감쌉니다.except ValueError본문은resolved를 참조합니다.Path(entry).resolve()가ValueError를 발생시키면resolved는 미할당 상태이며, 핸들러가UnboundLocalError로 실패합니다. 환경 변수에는 NUL 바이트가 들어갈 수 없으므로 현재 도달 가능성은 낮습니다. 그래도 두 단계를 분리하면 의도가 명확해지고 향후 리팩터링에 안전합니다.또한 저장소 내부 항목을 제외하는 로직이 "예외가 발생하지 않으면 아무 것도 하지 않는다"라는 암묵적 흐름에 의존합니다. 명시적 조건으로 바꾸면 가독성이 좋아집니다.
♻️ 제안 리팩터
- for entry in candidate_path.split(os.pathsep): - if not entry or not Path(entry).is_absolute(): - continue - try: - resolved = Path(entry).resolve() - resolved.relative_to(repo.resolve()) - except ValueError: - if str(resolved) not in safe_path_entries: - safe_path_entries.append(str(resolved)) - except OSError: - continue + repo_root = repo.resolve() + for entry in candidate_path.split(os.pathsep): + if not entry or not Path(entry).is_absolute(): + continue + try: + resolved = Path(entry).resolve() + except (OSError, ValueError): + continue + if resolved == repo_root or repo_root in resolved.parents: + continue + if str(resolved) not in safe_path_entries: + safe_path_entries.append(str(resolved))🤖 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 @.agents/skills/codex-review-loop/scripts/review_git_process.py around lines 144 - 160, In the PATH sanitization loop around safe_path_entries, separate Path(entry).resolve() from the resolved.relative_to(repo.resolve()) check so the exception handler never references an unassigned resolved value. Explicitly skip resolved paths inside the repository, and append only resolved paths outside it, while continuing to ignore OSError failures..agents/skills/codex-review-loop/tests/test_scope_security.py (1)
566-635: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsemantic 바이트 한도 상수를 import하여 픽스처 크기를 계산하십시오.
이 테스트들은
87_381,69_230,350_000,900_000같은 하드코딩된 반복 횟수를 사용합니다. 이 값들은review_scope_git.MAX_SEMANTIC_TOTAL_BYTES에 암묵적으로 결합되어 있습니다. 한도 상수를 변경하면 테스트가 실패하지만, 실패 원인이 크기 계산 때문인지 로직 회귀 때문인지 구분하기 어렵습니다.상수를 import하여 필요한 크기를 계산하면 의도가 드러나고 상수 변경에 자동으로 적응합니다.
♻️ 제안 리팩터 예시
def test_large_untracked_file_activates_specialists_conservatively(self) -> None: notes = self.repo / "docs" / "large.md" notes.parent.mkdir() + line = "ordinary text\n" + line_count = (review_scope_git.MAX_SEMANTIC_TOTAL_BYTES // len(line)) + 1 notes.write_text( - ("ordinary text\n" * 87_381) + "MainActor\n", + (line * line_count) + "MainActor\n", encoding="utf-8", )🤖 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 @.agents/skills/codex-review-loop/tests/test_scope_security.py around lines 566 - 635, Import review_scope_git.MAX_SEMANTIC_TOTAL_BYTES and replace the hardcoded fixture repetition counts and binary payload size in the affected tests with calculations derived from that limit, preserving each test’s intended over-budget behavior while making the fixture sizing adapt automatically when the limit changes.
🤖 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 @.agents/skills/codex-review-loop/scripts/lane_registry_validation.py:
- Around line 62-67: validate_registry_contract()에서 prompt_path의 Path.resolve()
호출을 try 블록 안으로 이동하고 RuntimeError를 기존 RegistryError 변환 대상에 추가하세요. 또한
resolve_review_scope.py의 load_registry()에서도 레지스트리 경로 resolve() 중 발생하는
RuntimeError를 ScopeError로 변환해 심볼릭 링크 루프가 내부 예외로 누출되지 않도록 하세요.
---
Nitpick comments:
In @.agents/skills/codex-review-loop/scripts/review_git_process.py:
- Around line 144-160: In the PATH sanitization loop around safe_path_entries,
separate Path(entry).resolve() from the resolved.relative_to(repo.resolve())
check so the exception handler never references an unassigned resolved value.
Explicitly skip resolved paths inside the repository, and append only resolved
paths outside it, while continuing to ignore OSError failures.
In @.agents/skills/codex-review-loop/tests/test_runner_contract.py:
- Around line 105-117: Update run_runner so use_default_codex=True removes any
inherited TOKI_REVIEW_CODEX_BIN from the copied environment before invoking the
runner; retain the fake-codex override when use_default_codex is false and
preserve explicit environment_overrides handling.
In @.agents/skills/codex-review-loop/tests/test_scope_security.py:
- Around line 566-635: Import review_scope_git.MAX_SEMANTIC_TOTAL_BYTES and
replace the hardcoded fixture repetition counts and binary payload size in the
affected tests with calculations derived from that limit, preserving each test’s
intended over-budget behavior while making the fixture sizing adapt
automatically when the limit changes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4cbb38db-0672-455a-81b8-7877989d7214
📒 Files selected for processing (16)
.agents/skills/codex-review-loop/SKILL.md.agents/skills/codex-review-loop/scripts/finding_grouping.py.agents/skills/codex-review-loop/scripts/finding_validation.py.agents/skills/codex-review-loop/scripts/lane_registry_validation.py.agents/skills/codex-review-loop/scripts/merge_findings.py.agents/skills/codex-review-loop/scripts/resolve_repo_root.py.agents/skills/codex-review-loop/scripts/resolve_review_scope.py.agents/skills/codex-review-loop/scripts/review_git_process.py.agents/skills/codex-review-loop/scripts/review_path_matching.py.agents/skills/codex-review-loop/scripts/review_scope_git.py.agents/skills/codex-review-loop/scripts/review_workspace.py.agents/skills/codex-review-loop/scripts/run_review_lane.sh.agents/skills/codex-review-loop/tests/test_finding_merge.py.agents/skills/codex-review-loop/tests/test_runner_contract.py.agents/skills/codex-review-loop/tests/test_scope_routing.py.agents/skills/codex-review-loop/tests/test_scope_security.py
🚧 Files skipped from review as they are similar to previous changes (2)
- .agents/skills/codex-review-loop/scripts/finding_validation.py
- .agents/skills/codex-review-loop/scripts/resolve_review_scope.py
| prompt_path = (skill_dir / prompt).resolve() | ||
| try: | ||
| prompt_path.relative_to(lanes_dir) | ||
| prompt_path.read_text(encoding="utf-8") | ||
| except (OSError, UnicodeError, ValueError) as error: | ||
| raise RegistryError(f"{lane_id}.prompt is not a readable lane prompt") from error |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
mkdir -p "$tmp/references/lanes"
ln -s "b.md" "$tmp/references/lanes/a.md"
ln -s "a.md" "$tmp/references/lanes/b.md"
python3 - "$tmp" <<'PY'
from pathlib import Path
import sys
path = Path(sys.argv[1]) / "references" / "lanes" / "a.md"
try:
path.resolve()
except RuntimeError:
print("symlink-loop resolution raises RuntimeError")
else:
raise SystemExit("expected RuntimeError")
PY
rg -n -C 5 'resolve\(\)' \
.agents/skills/codex-review-loop/scripts/lane_registry_validation.py \
.agents/skills/codex-review-loop/scripts/resolve_review_scope.pyRepository: choi138/toki
Length of output: 6353
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import sys
if sys.version_info < (3, 9):
print("python_version", sys.version)
print("sys.argv", sys.argv)
else:
print("python_version", sys.version)
print("sys.argv", sys.argv)
print("sys.version_info.minor", sys.version_info.minor)
PY
sed -n '1,90p' .agents/skills/codex-review-loop/scripts/lane_registry_validation.py
sed -n '55,95p' .agents/skills/codex-review-loop/scripts/resolve_review_scope.pyRepository: choi138/toki
Length of output: 4411
Path.resolve()의 RuntimeError도 경로 검증 예외로 처리하세요.
prompt_path = (skill_dir / prompt).resolve()가 try 블록 밖에 있으므로, 심볼릭 링크 루프가 있는 lane.prompt는 RegistryError 대신 누출되는 RuntimeError로 끝날 수 있습니다. validate_registry_contract()의 RuntimeError 처리도 추가하고, resolve_review_scope.py의 load_registry()도 Path.resolve() 호출 오류를 ScopeError로 변환하세요.
🤖 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 @.agents/skills/codex-review-loop/scripts/lane_registry_validation.py around
lines 62 - 67, validate_registry_contract()에서 prompt_path의 Path.resolve() 호출을
try 블록 안으로 이동하고 RuntimeError를 기존 RegistryError 변환 대상에 추가하세요. 또한
resolve_review_scope.py의 load_registry()에서도 레지스트리 경로 resolve() 중 발생하는
RuntimeError를 ScopeError로 변환해 심볼릭 링크 루프가 내부 예외로 누출되지 않도록 하세요.
Pick up the lane-based Codex review loop from #68 so the skill files are tracked here instead of lingering as a stale untracked copy.
1 participant
Summary
codex-review-loopskill with a baseline lane and seven specialist lanes.Why
Provide a repeatable local review workflow that keeps review read-only until fixes are explicitly approved, while leaving all GitHub writes outside the review loop.
Validation
git diff --checkand commit hooks passed.Summary by CodeRabbit