Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .jules/bolt.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
## 2024-06-21 - Python JSON Decoding Optimization
**Learning:** In Python, string slicing `text[index:]` inside a loop can cause O(N^2) complexity and severe memory copying overhead. When decoding JSON incrementally from a large text blob, `json.JSONDecoder().raw_decode(text, index)` can parse from a given index without slicing. Combining this with `text.find("{", index)` to skip irrelevant characters is significantly faster than `enumerate(text)`.
**Action:** Always prefer `raw_decode(text, index)` and `string.find()` over string slicing and character-by-character iteration when scanning large files for JSON objects.
## 2024-06-25 - Review and Merge Conflict Pre-checks
**Learning:** PRs cannot be merged if there are open reviewer change requests (including automated robot reviews) or merge conflicts. Continuous monitoring and resolution of base branch conflicts via `git fetch` and merge/rebase are essential to maintain a mergeable state.
**Action:** Always verify mergeability status and resolve reviewer requests or branch conflicts proactively before attempting to auto-merge PRs.
4 changes: 4 additions & 0 deletions .jules/sentinel.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,7 @@
**Vulnerability:** Workflow CI Security Bypass / Markdown Injection
**Learning:** The GitHub Actions workflow `opencode-review.yml` attempted to optimize performance by doing a fast-path bash string extraction. If this succeeded, it skipped the Python JSON normalizer (`opencode_review_normalize_output.py`). This is a security flaw because the bash script does not escape `<, >, &` characters, allowing attackers to inject `-->` directly in JSON strings to break out of HTML comment sections.
**Prevention:** Removed the fast-path check entirely. We must always enforce JSON normalization via `opencode_review_normalize_output.py` because it correctly parses the JSON payload and safely escapes all characters as `\u003c`, `\u003e` and `\u0026`.
## 2024-06-25 - Prevent CI Logs Security Exposure and Explicit Shell Usage
**Vulnerability:** Information Disclosure / Command Injection
**Learning:** `subprocess.run` defaults to `shell=False`, but linters like Bandit require explicit `shell=False` to pass security checks. Furthermore, logging `process.stderr` or command arguments in CI tools can leak sensitive data (e.g., GitHub tokens or API keys passed to commands) if a command fails and dumps the context.
**Prevention:** Always explicitly define `shell=False` when using `subprocess.run()`. Scrub secrets from both arguments and `stderr` before including them in error messages within CI scripts.
Binary file not shown.
Binary file not shown.
1 change: 1 addition & 0 deletions scripts/ci/opencode_review_approve_gate.sh
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ def changed_new_lines(path_value: str) -> set[int]:
],
check=False,
text=True,
shell=False,
stdout=subprocess.PIPE,
stderr=subprocess.DEVNULL,
)
Expand Down
10 changes: 7 additions & 3 deletions scripts/ci/pr_review_merge_scheduler.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,15 @@ class Decision:


def run(args: list[str], *, stdin: str | None = None) -> str:
"""Run a command and return stdout, raising with stderr on failure."""
process = subprocess.run(args, input=stdin, capture_output=True, text=True)
"""Run a command and return stdout, raising with scrubbed args and stderr on failure."""
import re
process = subprocess.run(args, input=stdin, capture_output=True, text=True, shell=False)
if process.returncode != 0:
pattern = r'(?i)(gh[psuo]_[a-zA-Z0-9]{36}|bearer\s+[a-zA-Z0-9_\-]+|token\s+[a-zA-Z0-9_\-]+)'
Comment thread
github-actions[bot] marked this conversation as resolved.
Outdated
args_scrubbed = re.sub(pattern, '***', ' '.join(args))
stderr_scrubbed = re.sub(pattern, '***', process.stderr or "")
raise RuntimeError(
f"Command failed ({process.returncode}): {' '.join(args)}\n{process.stderr}"
f"Command failed ({process.returncode}): {args_scrubbed}\n{stderr_scrubbed}"
)
return process.stdout

Expand Down
Binary file not shown.
Binary file not shown.
27 changes: 27 additions & 0 deletions tests/test_pr_review_merge_scheduler.py
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,33 @@ def test_review_state_and_failed_checks():
assert sched.failed_status_checks(failed) == ["strix", "lint"]


def test_run_command_failure_scrubs_secrets(monkeypatch):
import subprocess
class MockProcess:
def __init__(self, returncode, stdout, stderr):
self.returncode = returncode
self.stdout = stdout
self.stderr = stderr

def mock_run(args, **kwargs):
stderr_msg = "Error using Token secret_token_123 and bearer super_secret"
if "fail" in args:
return MockProcess(1, "", stderr_msg)
return MockProcess(0, "success", "")

monkeypatch.setattr(subprocess, "run", mock_run)

assert sched.run(["success"]) == "success"

with pytest.raises(RuntimeError) as exc_info:
sched.run(["gh", "api", "fail", "-H", "Authorization: token ghp_123456789012345678901234567890123456"])

error_msg = str(exc_info.value)
assert "ghp_123456789012345678901234567890123456" not in error_msg
Comment thread
github-actions[bot] marked this conversation as resolved.
Outdated
assert "secret_token_123" not in error_msg
assert "super_secret" not in error_msg
assert "***" in error_msg

def test_actions_call_gh_with_expected_arguments(monkeypatch):
calls = []
monkeypatch.setattr(sched, "run", lambda args: calls.append(args) or "")
Expand Down
Loading