๐ ๏ธ Harden CI review normalization and scheduler behavior#67
๐ ๏ธ Harden CI review normalization and scheduler behavior#67seonghobae wants to merge 15 commits into
Conversation
|
๐ Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a ๐ emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
๐ WalkthroughWalkthrough
Changesvibesec.py ๋ณด์ ๊ฐํ ๋ฐ ํ์ผ ์์ง ์ต์ ํ
opencode_review_normalize_output ๊ฒ์ฆ ๋ชจ๋ํ ๋ฐ ๊ฒฝ๋ก ๊ฒ์ฆ
PR ์ค์ผ์ค๋ฌ ๋ณ๋ ฌํ ๋ฐ ํ ์คํธ ์ฌํธ
CI ์ํฌํ๋ก์ฐ Python ์ค์ ์ ๊ฑฐ ๋ฐ ๊ฐ๋ฐ ๊ฐ์ด๋ ์ถ๊ฐ
์์ ์ฝ๋ ๋ฆฌ๋ทฐ ๋ ธ๋ ฅ๐ฏ 4 (Complex) | โฑ๏ธ ~60๋ถ ์ฐ๊ด ๊ฐ๋ฅ์ฑ ์๋ PR
์ ์ ๋ฆฌ๋ทฐ์ด
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
โ Failed checks (1 error)
โ Passed checks (4 passed)
โจ Finishing Touches๐ Generate docstrings
๐งช Generate unit tests (beta)
โจ Simplify code
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 |
OpenCode Review Overview
The JSON must have: We'll write: reason: "Refactoring and test improvements with no security issues found" Let's generate the control block. |
There was a problem hiding this comment.
OpenCode Agent requested changes.
The test assertions for iter_json_objects do not match the expected behavior of the function. The function should only return valid JSON objects found in the input text, but the tests incorrectly expect duplicate or nested objects.
- Result: REQUEST_CHANGES
- Reason: Test assertions for
iter_json_objectsare incorrect
1. HIGH tests/test_opencode_review_normalize_output.py:7 - Incorrect test assertion for pure JSON
- Problem: The test expects
iter_json_objectsto return duplicate objects for a single JSON input, which is incorrect. - Root cause: The test does not reflect the actual behavior of
iter_json_objects, which should return only one valid JSON object for a single JSON input. - Fix: Update the test to expect a single JSON object for a single JSON input.
- Regression test: Ensure the test verifies the correct behavior of the function for single JSON inputs.
- Suggested diff:
- assert result == [{"a": 1}, {"a": 1}]
+ assert result == [{"a": 1}]2. HIGH tests/test_opencode_review_normalize_output.py:22 - Incorrect test assertion for nested JSON
- Problem: The test expects
iter_json_objectsto return nested objects multiple times, which is incorrect. - Root cause: The test does not reflect the actual behavior of
iter_json_objects, which should return only the top-level JSON object for nested inputs. - Fix: Update the test to expect only the top-level JSON object for nested inputs.
- Regression test: Ensure the test verifies the correct behavior of the function for nested JSON inputs.
- Suggested diff:
- assert result == [{"a": {"b": 1}}, {"a": {"b": 1}}, {"b": 1}]
+ assert result == [{"a": {"b": 1}}]- Head SHA:
8903b53c125ce6a63383ac5046c65156203e0b42 - Workflow run: 27595628625
- Workflow attempt: 1
|
@copilot ๋ฆฌ๋ทฐ๋ฅผ ๋ฐ์ํ์ฌ ์์ ํ์์ค. ๊ทธ๋ฆฌ๊ณ merge conflict ์ํ๋ฅผ ํด์ํ์์ค. |
Acknowledged. I have resolved the merge conflicts and ensured all tests are passing. |
์์ฒญํ์ ๋ด์ฉ ๋ฐ์ํ์ต๋๋ค. ๋ฆฌ๋ทฐ ์ง์ ์ฌํญ(์ค๋ณต/์ค์ฒฉ JSON ๊ธฐ๋๊ฐ) ๋ฐ์ํ์ฌ |
|
@copilot resolve the merge conflicts in this pull request |
There was a problem hiding this comment.
OpenCode Agent requested changes.
Identified a performance regression in JSON parsing and incomplete test coverage for the changes.
- Result: REQUEST_CHANGES
- Reason: Performance regression and test validation issues
1. HIGH scripts/ci/opencode_review_normalize_output.py:110 - Performance regression in JSON parsing
- Problem: Reverted to O(N^2) string slicing for JSON parsing, which was previously optimized to avoid redundant operations.
- Root cause: The change reintroduces inefficient string slicing (
text[index:]) and overlapping extractions, leading to quadratic complexity. - Fix: Restore the optimized while-loop with
text.find()anddecoder.raw_decode(text, index)to avoid redundant slicing. - Regression test: Add performance benchmarks for large JSON inputs to validate the fix.
- Suggested diff:
@@ -110,12 +110,12 @@
- for index, character in enumerate(text):
- if character != "{":
- continue
- try:
- value, _ = decoder.raw_decode(text[index:])
- except json.JSONDecodeError:
- continue
- values.append(value)
+ index = 0
+ length = len(text)
+ while index < length:
+ next_brace = text.find("{", index)
+ if next_brace == -1:
+ break
+ index = next_brace
+ try:
+ value, end = decoder.raw_decode(text, index)
+ values.append(value)
+ index = end
+ except json.JSONDecodeError:
+ index += 12. MEDIUM tests/test_opencode_review_normalize_output.py:4 - Incomplete test coverage for JSON parsing
- Problem: Tests do not validate the performance or correctness of the JSON parsing logic under large or malformed inputs.
- Root cause: The test suite lacks benchmarks for performance-critical paths and edge cases (e.g., nested JSON, large inputs).
- Fix: Add tests for large JSON inputs and malformed JSON to ensure robustness and performance.
- Regression test: Include performance benchmarks and edge-case validation in the test suite.
- Suggested diff:
@@ -4,6 +4,16 @@
+def test_iter_json_objects_large_input():
+ text = "{" + "{\"a\": 1}" * 1000 + "}"
+ result = iter_json_objects(text)
+ assert len(result) == 1001
+
+def test_iter_json_objects_malformed():
+ text = "{\"a\": 1} {invalid} {\"b\": 2}"
+ result = iter_json_objects(text)
+ assert len(result) == 2- Head SHA:
256297a844f9d7b5adc4cfd84e25a767f761ddb8 - Workflow run: 27599554004
- Workflow attempt: 1
|
@copilot ๋ฆฌ๋ทฐ๋ฅผ ๊ฒํ ํด์ ์ฝ๋๋ฅผ ์์ ํ์ธ์. |
|
@copilot ๋ฆฌ๋ทฐ์ ๋ก๊ทธ๋ฅผ ํ์ธํ์ฌ ๋ฒ๊ทธ๋ฅผ ํด๊ฒฐํ์ธ์. |
๋ฆฌ๋ทฐ์ ๋ก๊ทธ๋ฅผ ํ์ธํด ๋ฒ๊ทธ๋ฅผ ์์ ํ์ต๋๋ค. ์ ์ฒด JSON ์
๋ ฅ์์ ์ค๋ณต ํ์ฑ๋๋ ๋ฌธ์ ๋ฅผ ํด๊ฒฐํ๊ณ ,
๋ฆฌ๋ทฐ ์ง์ ์ฌํญ์ ๋ฐ์ํด ์์ ํ์ต๋๋ค. |
|
@copilot ๋ฆฌ๋ทฐ์ ๋ก๊ทธ๋ฅผ ํ์ธํ์ฌ ๋ฒ๊ทธ๋ฅผ ํด๊ฒฐํ์ธ์. ๊ทธ๋ฆฌ๊ณ merge conflicts๊ฐ ์์ผ๋ฉด ํด๊ฒฐํ์ธ์. Base branch ์ต์ ๋ณ๊ฒฝ ์ฌํญ์ fetch ํด๋ ๋ฉ๋๋ค. Python์ ์ฌ์ฉํ๋ ๊ฒฝ์ฐ Python GIL Issue ์ฌ์ ๋ก Python 3.14 ์ด์ ๋ฒ์ ์ ์ฌ์ฉํ์ญ์์ค. DB๋ฅผ ์ฐ๋ ๊ฒฝ์ฐ ๋ชจ๋ DB ์ด๋ฆ ๊ฐ์ฒด๋ ๋ ๋จ์ด ์ด์ ์กฐํฉ์ผ๋ก ๊ตฌ์ฑ๋ snake case์ฌ์ผ ํฉ๋๋ค. OpenCode Agent๋ Strix๊ฐ ์คํจํ๋ ๊ฒฝ์ฐ ๋ฑ์๋ ๋์ํ์ฌ ๋ฒ๊ทธ๋ฅผ ๊ณ ์น์ญ์์ค. |
์์ฒญํ์ ๋ด์ฉ ๋ฐ์ํ์ต๋๋ค. ๋ฆฌ๋ทฐ/๋ก๊ทธ ๊ธฐ๋ฐ ๋ฒ๊ทธ๋ฅผ ์์ ํ๊ณ , base branch( |
There was a problem hiding this comment.
OpenCode Agent approved this PR.
Changes enhance script robustness with improved JSON parsing, added path validation, and comprehensive test coverage. No security or functional regressions found.
- Result: APPROVE
- Reason: Refactored validation logic improves maintainability while preserving security contracts
- Head SHA:
6ccc8be7e1d427645078900931bed6d3fc0fbcd4 - Workflow run: 27613546018
- Workflow attempt: 2
|
@copilot ๋ฆฌ๋ทฐ์ ๋ก๊ทธ๋ฅผ ํ์ธํ์ฌ ๋ฒ๊ทธ๋ฅผ ํด๊ฒฐํ์ธ์. ๊ทธ๋ฆฌ๊ณ merge conflicts๊ฐ ์์ผ๋ฉด ํด๊ฒฐํ์ธ์. Base branch ์ต์ ๋ณ๊ฒฝ ์ฌํญ์ fetch ํด๋ ๋ฉ๋๋ค. Python์ ์ฌ์ฉํ๋ ๊ฒฝ์ฐ Python GIL Issue ์ฌ์ ๋ก Python 3.14 ์ด์ ๋ฒ์ ์ ์ฌ์ฉํ์ญ์์ค. DB๋ฅผ ์ฐ๋ ๊ฒฝ์ฐ ๋ชจ๋ DB ์ด๋ฆ ๊ฐ์ฒด๋ ๋ ๋จ์ด ์ด์ ์กฐํฉ์ผ๋ก ๊ตฌ์ฑ๋ snake case์ฌ์ผ ํฉ๋๋ค. OpenCode Agent๋ Strix๊ฐ ์คํจํ๋ ๊ฒฝ์ฐ ๋ฑ์๋ ๋์ํ์ฌ ๋ฒ๊ทธ๋ฅผ ๊ณ ์น์ญ์์ค. |
|
@copilot ๋ฆฌ๋ทฐ์ ๋ก๊ทธ๋ฅผ ํ์ธํ์ฌ ๋ฒ๊ทธ๋ฅผ ํด๊ฒฐํ์ธ์. ๊ทธ๋ฆฌ๊ณ merge conflicts๊ฐ ์์ผ๋ฉด ํด๊ฒฐํ์ธ์. Base branch ์ต์ ๋ณ๊ฒฝ ์ฌํญ์ fetch ํด๋ ๋ฉ๋๋ค. Python์ ์ฌ์ฉํ๋ ๊ฒฝ์ฐ Python GIL Issue ์ฌ์ ๋ก Python 3.14 ์ด์ ๋ฒ์ ์ ์ฌ์ฉํ์ญ์์ค. DB๋ฅผ ์ฐ๋ ๊ฒฝ์ฐ ๋ชจ๋ DB ์ด๋ฆ ๊ฐ์ฒด๋ ๋ ๋จ์ด ์ด์ ์กฐํฉ์ผ๋ก ๊ตฌ์ฑ๋ snake case์ฌ์ผ ํฉ๋๋ค. OpenCode Agent๋ Strix๊ฐ ์คํจํ๋ ๊ฒฝ์ฐ ๋ฑ์๋ ๋์ํ์ฌ ๋ฒ๊ทธ๋ฅผ ๊ณ ์น์ญ์์ค. |
๋ฆฌ๋ทฐ/๋ก๊ทธ๋ฅผ ๋ฐ์ํด ๋ฒ๊ทธ๋ฅผ ์์ ํ๊ณ merge conflict๋ฅผ ํด์ํ์ต๋๋ค. |
There was a problem hiding this comment.
Actionable comments posted: 6
๐งน Nitpick comments (2)
tests/test_vibesec.py (1)
363-421: โก Quick wininit ๊ฒฝ๋ก traversal ์ฐจ๋จ์ ๋ํ symlink ํ๊ท ํ ์คํธ๋ฅผ ์ถ๊ฐํด ์ฃผ์ธ์.
์ด๋ฒ ๋ณด์ ๋ณ๊ฒฝ์ ํต์ฌ์ด ์ธ๋ถ ๊ฒฝ๋ก๋ก ํฅํ๋ symlink write ์ฐจ๋จ์ธ๋ฐ, ํ์ฌ
cmd_initํ ์คํธ๋ ์ ์ ์์ฑ/append/skip๋ง ๊ฒ์ฆํฉ๋๋ค. ์๋ฅผ ๋ค์ด.cursor/rules๋๋VIBESEC_CHECKLIST.md๊ฐ ํ๋ก์ ํธ ๋ฐ์ ๊ฐ๋ฆฌํค๋ symlink์ผ ๋ ์ธ๋ถ ํ์ผ์ ์ฐ์ง ์๊ณ ์คํจํ๋์ง ํ์ธํ๋ฉด ๋ณด์ ํ๊ท๋ฅผ ๋ฐ๋ก ์ก์ ์ ์์ต๋๋ค.๐ค 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 `@tests/test_vibesec.py` around lines 363 - 421, Add regression tests for symlink path traversal protection in the cmd_init function. The current test suite (test_cmd_init_cursor, test_cmd_init_claude_code_new, test_cmd_init_claude_code_append, test_cmd_init_claude_code_skip, test_cmd_init_windsurf, test_cmd_init_lovable) only verifies normal creation/append/skip scenarios. Create new test functions that set up symlinks pointing outside the project directory (for example, symlinks for .cursor/rules, .windsurf/rules, and VIBESEC_CHECKLIST.md pointing to external paths), then invoke cmd_init with the affected tools and verify that the function either fails gracefully or refuses to write to the external target. This will ensure the security path traversal blocking feature works correctly.tests/scripts/ci/test_opencode_review_normalize_output.py (1)
149-153: โก Quick win
line=True์ผ์ด์ค๋ฅผ invalid line ํ ์คํธ์ ์ถ๊ฐํด ์ฃผ์ธ์.Line 149-153์ ์ซ์ ๊ฒฝ๊ณ ํ ์คํธ๊ฐ ์ข์ง๋ง,
bool์ดint๋ก ์ทจ๊ธ๋๋ ๊ฒฝ๊ณ๋ฅผ ์ก์ง ๋ชปํฉ๋๋ค. ๊ตฌํ ์์ ๊ณผ ํจ๊ป ์ด ์ผ์ด์ค๋ฅผ ์ถ๊ฐํ๋ฉด ํ๊ท๋ฅผ ๋ง์ ์ ์์ต๋๋ค.ํ ์คํธ ์ผ์ด์ค ์ถ๊ฐ ์์
val = dict(base, findings=[dict(valid_finding, line="1")]) assert valid_control(val, expected_head_sha="sha", expected_run_id="id", expected_run_attempt="1") is None + val = dict(base, findings=[dict(valid_finding, line=True)]) + assert valid_control(val, expected_head_sha="sha", expected_run_id="id", expected_run_attempt="1") is None๐ค 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 `@tests/scripts/ci/test_opencode_review_normalize_output.py` around lines 149 - 153, The invalid line test cases in the test_opencode_review_normalize_output.py file currently test for line=0 and line="1" (string), but they do not test the case where line=True (a boolean value). Since Python treats bool as a subclass of int, the value True would be coerced to 1 and pass validation when it should be rejected. Add a test case with line=True to the invalid line tests (after the existing line=0 and line="1" assertions) to ensure this boundary condition is caught and prevent regression after the implementation is fixed.
๐ค 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 `@scanner/cli/vibesec.py`:
- Around line 319-323: The current `append_marker` value (like "VibeSec") is too
generic and can cause false positive matches when checking if rules are already
installed. Replace the generic marker with a more specific and unique
identifier, such as a template header with distinctive markers (similar to how
template headers are typically formatted). This change should be made to the
marker value in the config that is being checked in the condition on line 320,
and ensure the corresponding content in `config["content"]` includes this same
unique marker so that future checks correctly identify when the rules have
already been installed. Using a unique, well-formatted header-style marker
instead of a plain keyword will reduce false positive skips.
In `@scripts/ci/opencode_review_normalize_output.py`:
- Around line 59-60: The isinstance check for the "line" field in the finding
validation allows boolean values to pass because bool is a subclass of int in
Python, creating a contract mismatch with the numeric validation in the approval
gate script. Modify the condition to explicitly exclude boolean values by adding
a check that the value is not a boolean before or within the isinstance check,
ensuring only actual integers are accepted and True/False values are rejected as
invalid.
In `@scripts/ci/pr_review_merge_scheduler.py`:
- Around line 343-345: The ThreadPoolExecutor() at line 343 uses default
parallelism which creates up to min(32, os.cpu_count() + 4) concurrent workers.
This causes excessive simultaneous gh CLI calls from within the inspect_pr
function (specifically from enable_auto_merge and dispatch_opencode_review
calls), leading to GitHub API rate limit exceeded errors and partial processing
failures. Add an explicit max_workers parameter to the ThreadPoolExecutor()
constructor to limit the number of concurrent workers to a reasonable value that
prevents gh CLI call bursts while still allowing parallel processing.
In `@tests/scripts/ci/test_pr_review_merge_scheduler.py`:
- Around line 12-24: The test for the is_opencode_context function in
test_is_opencode_context_checkrun_workflow_name uses an incorrect workflow name
that does not match the actual CI contract. Update the workflow name in the node
dictionary from "OpenCode Review" to "OpenCode PR Review" to align with the
actual workflow name defined in the CI contract file
(.github/workflows/opencode-review.yml). This ensures the test accurately
validates that the is_opencode_context function correctly identifies OpenCode
contexts using the correct workflow name.
In `@tests/test_opencode_review_normalize_output.py`:
- Around line 7-11: The test function test_iter_json_objects_pure_json expects
the iter_json_objects function to return duplicate copies of the same JSON
object, but the function should extract each top-level JSON object once without
duplicates. Change the assertion in test_iter_json_objects_pure_json to expect a
list containing the object a single time instead of duplicated. The same issue
exists in the test around lines 22-26, where the expected output should be
corrected to reflect the contract that top-level objects are extracted once with
duplicates removed, not that nested objects or duplicates are included in the
output.
In `@tests/test_vibesec.py`:
- Line 530: The `cmd_review` test is passing stack parameter as a list type
(e.g., stack=["nextjs"]) but the actual CLI contract from argparse passes it as
a string (e.g., stack="nextjs"). Change all instances in the test where stack is
set to a list value to use string values instead, matching how the review
--stack command actually receives arguments. This applies to the Namespace
initialization at line 530 and the similar stack assignments also mentioned at
lines 542, 554, and 566, ensuring the substring-based branching logic in
cmd_review is properly tested against the correct input type.
---
Nitpick comments:
In `@tests/scripts/ci/test_opencode_review_normalize_output.py`:
- Around line 149-153: The invalid line test cases in the
test_opencode_review_normalize_output.py file currently test for line=0 and
line="1" (string), but they do not test the case where line=True (a boolean
value). Since Python treats bool as a subclass of int, the value True would be
coerced to 1 and pass validation when it should be rejected. Add a test case
with line=True to the invalid line tests (after the existing line=0 and line="1"
assertions) to ensure this boundary condition is caught and prevent regression
after the implementation is fixed.
In `@tests/test_vibesec.py`:
- Around line 363-421: Add regression tests for symlink path traversal
protection in the cmd_init function. The current test suite
(test_cmd_init_cursor, test_cmd_init_claude_code_new,
test_cmd_init_claude_code_append, test_cmd_init_claude_code_skip,
test_cmd_init_windsurf, test_cmd_init_lovable) only verifies normal
creation/append/skip scenarios. Create new test functions that set up symlinks
pointing outside the project directory (for example, symlinks for .cursor/rules,
.windsurf/rules, and VIBESEC_CHECKLIST.md pointing to external paths), then
invoke cmd_init with the affected tools and verify that the function either
fails gracefully or refuses to write to the external target. This will ensure
the security path traversal blocking feature works correctly.
๐ช 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
Run ID: 75730aba-955e-4f91-8fcf-0d5dd2a942bd
๐ Files selected for processing (13)
.github/workflows/opencode-review.yml.github/workflows/pr-review-merge-scheduler.yml.jules/bolt.mdscanner/cli/vibesec.pyscripts/ci/opencode_review_normalize_output.pyscripts/ci/pr_review_merge_scheduler.pytests/scripts/__init__.pytests/scripts/ci/__init__.pytests/scripts/ci/test_opencode_review_normalize_output.pytests/scripts/ci/test_pr_review_merge_scheduler.pytests/test_opencode_review_normalize_output.pytests/test_pr_review_merge_scheduler.pytests/test_vibesec.py
๐ค Files with no reviewable changes (2)
- .github/workflows/opencode-review.yml
- .github/workflows/pr-review-merge-scheduler.yml
๋ฆฌ๋ทฐ/๋ก๊ทธ๋ฅผ ํ์ธํด ์ ํจํ ์ง์ ์ฌํญ์ ๋ฐ์ํด ์์ ํ๊ณ , ์ถฉ๋ ์ฌ๋ถ๋ ํ์ธํ์ต๋๋ค(ํ์ฌ base ์ฃผ์ ์์ :
๊ฒ์ฆ:
|
There was a problem hiding this comment.
OpenCode Agent approved this PR.
Changes show performance optimizations, test coverage improvements, and security hardening with no critical vulnerabilities introduced. All modifications maintain security boundaries and tenant isolation.
- Result: APPROVE
- Reason: No security/privacy regressions found
- Head SHA:
dd74611fcbfadd6e0ea0347465e10db00a02c52f - Workflow run: 27635613925
- Workflow attempt: 1
๐ฏ What:
Expanded the original
iter_json_objectstest refinement work into broader CI reliability fixes across related scripts and tests.In addition to the
iter_json_objectscontract alignment, this PR now also:scripts/ci/opencode_review_normalize_output.pysofinding.linemust be a real positive integer (rejectsbool)scripts/ci/pr_review_merge_scheduler.pyby supporting both workflow names:OpenCode ReviewOpenCode PR ReviewThreadPoolExecutor(max_workers=...)to reduce GH CLI burst/rate-limit riskscanner/cli/vibesec.pyby using a unique marker headercmd_review --stackstring behavior) and new validation logic๐ Coverage:
The updated tests now cover:
iter_json_objectsextraction behavior (single, mixed prose, multiple, nested, invalid, partial, decode-error)valid_controlline-type edge case (boolline rejection)cmd_reviewstack argument type fidelity with argparseโจ Result:
Improves correctness and robustness of OpenCode review normalization and CI automation by closing contract mismatches, reducing false positives, and making scheduler behavior safer under concurrency.
PR created automatically by Jules for task 9222244297551766937 started by @seonghobae