Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
🤖 CI report🚨 Trunk lane — universal laneThis PR is assigned to the universal lane. It cannot merge in parallel with other PRs, so it can take longer to merge. Ask dev-ex if you think this is wrong. ✅ Duplication (Python) — cleanNew Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying. ✅ Duplication (TypeScript) — cleanNew TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.
|
| File | Comment lines | Added lines |
|---|---|---|
tools/hogli-commands/hogli_commands/preflight_checks.py |
24 | 262 |
tools/hogli-commands/hogli_commands/ci_preflight.py |
6 | 71 |
tools/hogli-commands/hogli_commands/tests/test_ci_preflight.py |
1 | 44 |
This check does not block merging. It updates on every push and clears when the share drops.
|
[High risk] Adds new preflight checks that run custom code on every push. The PR appears safe to merge based on this review; no new actionable finding remains. Reviews (2) · Last reviewed commit: "chore(ci): run preflight semgrep from pa..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Mirrors lint-staged's `format:js`, which agents bypass via --no-verify. | ||
| verify=["pnpm", "exec", "oxfmt", "--check", "--no-error-on-unmatched-pattern"], | ||
| fix=["pnpm", "exec", "oxfmt", "--no-error-on-unmatched-pattern"], | ||
| requires=("node",), | ||
| takes_files=True, | ||
| ), |
There was a problem hiding this comment.
--strict mode, preflight selects committed changed paths, but this file-taking check runs oxfmt on their working-tree contents. An uncommitted formatting edit can block a push even though that edit is not being pushed. Check committed copies in strict mode, or make working-tree formatting advisory there.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tools/hogli-commands/hogli_commands/ci_preflight.py
Line: 246-251
Comment:
**Uncommitted formatting blocks pushes.** In `--strict` mode, preflight selects committed changed paths, but this file-taking check runs oxfmt on their working-tree contents. An uncommitted formatting edit can block a push even though that edit is not being pushed. Check committed copies in strict mode, or make working-tree formatting advisory there.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: PostHog/posthog/.coderabbit.yaml Review profile: QUIET Plan: Enterprise Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe CI preflight runner adds an Oxfmt check for selected frontend files and runs registered diff-based checks for snapshot baseline removals, newly introduced Semgrep findings, and merge-queue lane selection. The checks compare changes against a merge base and report pass, fail, warning, advisory, or skipped outcomes. Tests cover check behavior and runner handling. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to A removed baseline for an unchanged story can go unflagged and cause a merge-queue failure. Correct the snapshot check before merging unless that risk is explicitly accepted. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change adds local developer checks without demonstrating new production access or weaker CI enforcement. Incomplete scans are identified as skipped rather than clean. Remaining uncertainty concerns complete security coverage and cleanup after forced process termination. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 76b88204-9a99-4818-9138-4ae228ea9f87
📒 Files selected for processing (5)
.agents/skills/running-ci-preflight/SKILL.mdtools/hogli-commands/hogli_commands/ci_preflight.pytools/hogli-commands/hogli_commands/preflight_checks.pytools/hogli-commands/hogli_commands/tests/test_ci_preflight.pytools/hogli-commands/hogli_commands/tests/test_preflight_checks.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
| def _semgrep_findings(semgrep: list[str], root: Path) -> dict[Finding, list[int]] | None: | ||
| """Findings under *root*, each with the lines it starts on. None when the scan did not run. | ||
|
|
||
| The target is the directory and not the files in it. Semgrep applies its default | ||
| ignore list (``tests/``, ``node_modules/``, minified files) to a directory it walks, | ||
| which is how CI scans, and skips that list for a file named on the command line. | ||
| """ | ||
| try: | ||
| result = subprocess.run( | ||
| [ | ||
| *semgrep, | ||
| "--config", | ||
| str(REPO_ROOT / SEMGREP_RULES), | ||
| "--severity=WARNING", | ||
| "--severity=ERROR", | ||
| "--metrics=off", | ||
| "--quiet", | ||
| "--json", | ||
| ".", | ||
| ], | ||
| cwd=root, | ||
| env={**os.environ, "SEMGREP_ENABLE_VERSION_CHECK": "false"}, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=_SEMGREP_TIMEOUT_SECONDS, | ||
| ) | ||
| findings: dict[Finding, list[int]] = {} | ||
| sources: dict[str, list[str]] = {} | ||
| for item in json.loads(result.stdout)["results"]: | ||
| path = item["path"] | ||
| if path not in sources: | ||
| sources[path] = (root / path).read_text(errors="replace").splitlines() | ||
| start, end = item["start"]["line"], item["end"]["line"] | ||
| matched = "\n".join(line.strip() for line in sources[path][start - 1 : end]) | ||
| # The id prefix encodes the rule file's path relative to the working directory. | ||
| # The last segment is the rule's own id. | ||
| rule = item["check_id"].rsplit(".", 1)[-1] | ||
| findings.setdefault((rule, path, matched), []).append(start) | ||
| except (OSError, subprocess.TimeoutExpired, ValueError, KeyError, TypeError): | ||
| # A missing uvx download, a crash and a timeout all end here, because none of them prints a report. | ||
| return None | ||
| return findings |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🌐 Web query:
semgrep CLI exit codes json errors field
💡 Result:
`semgrep scan --json` puts execution errors in the top-level `errors` array; it is separate from `results`, which contains findings. Each error has required `code`, `level`, and `type` fields. Optional fields include `message`, `path`, `rule_id`, `long_msg`, and `short_msg`. The schema describes `code` as the exit code associated with that error type—not necessarily the process’s overall exit status. ([github.com](https://github.com/semgrep/semgrep-interfaces/blob/main/semgrep_output_v1.jsonschema?utm_source=openai))
For the process status, check the CLI’s actual exit code; don’t infer it from `errors` alone. The schema doesn’t establish a universal mapping between individual JSON errors and the overall CLI exit status, and behavior may depend on CLI version and flags. ([github.com](https://github.com/semgrep/semgrep-interfaces/blob/main/semgrep_output_v1.jsonschema?utm_source=openai))
Citations:
- 1: https://github.com/semgrep/semgrep-interfaces/blob/main/semgrep_output_v1.jsonschema?utm_source=openai
- 2: https://github.com/semgrep/semgrep-interfaces/blob/main/semgrep_output_v1.jsonschema?utm_source=openai
Treat Semgrep execution errors as an unavailable scan.
_semgrep_findings reads only results. A Semgrep process can return JSON with an empty results array and execution errors in the top-level errors array. The caller can then treat the scan as clean and miss a new finding.
Check both the process status and report["errors"]. Preserve the valid Semgrep finding exit code for the configured command.
Suggested fix
- findings: dict[Finding, list[int]] = {}
+ if result.returncode not in (0, 1):
+ return None
+ report = json.loads(result.stdout)
+ if report.get("errors"):
+ return None
+ findings: dict[Finding, list[int]] = {}
sources: dict[str, list[str]] = {}
- for item in json.loads(result.stdout)["results"]:
+ for item in report["results"]:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _semgrep_findings(semgrep: list[str], root: Path) -> dict[Finding, list[int]] | None: | |
| """Findings under *root*, each with the lines it starts on. None when the scan did not run. | |
| The target is the directory and not the files in it. Semgrep applies its default | |
| ignore list (``tests/``, ``node_modules/``, minified files) to a directory it walks, | |
| which is how CI scans, and skips that list for a file named on the command line. | |
| """ | |
| try: | |
| result = subprocess.run( | |
| [ | |
| *semgrep, | |
| "--config", | |
| str(REPO_ROOT / SEMGREP_RULES), | |
| "--severity=WARNING", | |
| "--severity=ERROR", | |
| "--metrics=off", | |
| "--quiet", | |
| "--json", | |
| ".", | |
| ], | |
| cwd=root, | |
| env={**os.environ, "SEMGREP_ENABLE_VERSION_CHECK": "false"}, | |
| capture_output=True, | |
| text=True, | |
| timeout=_SEMGREP_TIMEOUT_SECONDS, | |
| ) | |
| findings: dict[Finding, list[int]] = {} | |
| sources: dict[str, list[str]] = {} | |
| for item in json.loads(result.stdout)["results"]: | |
| path = item["path"] | |
| if path not in sources: | |
| sources[path] = (root / path).read_text(errors="replace").splitlines() | |
| start, end = item["start"]["line"], item["end"]["line"] | |
| matched = "\n".join(line.strip() for line in sources[path][start - 1 : end]) | |
| # The id prefix encodes the rule file's path relative to the working directory. | |
| # The last segment is the rule's own id. | |
| rule = item["check_id"].rsplit(".", 1)[-1] | |
| findings.setdefault((rule, path, matched), []).append(start) | |
| except (OSError, subprocess.TimeoutExpired, ValueError, KeyError, TypeError): | |
| # A missing uvx download, a crash and a timeout all end here, because none of them prints a report. | |
| return None | |
| return findings | |
| def _semgrep_findings(semgrep: list[str], root: Path) -> dict[Finding, list[int]] | None: | |
| """Findings under *root*, each with the lines it starts on. None when the scan did not run. | |
| The target is the directory and not the files in it. Semgrep applies its default | |
| ignore list (``tests/``, ``node_modules/``, minified files) to a directory it walks, | |
| which is how CI scans, and skips that list for a file named on the command line. | |
| """ | |
| try: | |
| result = subprocess.run( | |
| [ | |
| *semgrep, | |
| "--config", | |
| str(REPO_ROOT / SEMGREP_RULES), | |
| "--severity=WARNING", | |
| "--severity=ERROR", | |
| "--metrics=off", | |
| "--quiet", | |
| "--json", | |
| ".", | |
| ], | |
| cwd=root, | |
| env={**os.environ, "SEMGREP_ENABLE_VERSION_CHECK": "false"}, | |
| capture_output=True, | |
| text=True, | |
| timeout=_SEMGREP_TIMEOUT_SECONDS, | |
| ) | |
| if result.returncode not in (0, 1): | |
| return None | |
| report = json.loads(result.stdout) | |
| if report.get("errors"): | |
| return None | |
| findings: dict[Finding, list[int]] = {} | |
| sources: dict[str, list[str]] = {} | |
| for item in report["results"]: | |
| path = item["path"] | |
| if path not in sources: | |
| sources[path] = (root / path).read_text(errors="replace").splitlines() | |
| start, end = item["start"]["line"], item["end"]["line"] | |
| matched = "\n".join(line.strip() for line in sources[path][start - 1 : end]) | |
| # The id prefix encodes the rule file's path relative to the working directory. | |
| # The last segment is the rule's own id. | |
| rule = item["check_id"].rsplit(".", 1)[-1] | |
| findings.setdefault((rule, path, matched), []).append(start) | |
| except (OSError, subprocess.TimeoutExpired, ValueError, KeyError, TypeError): | |
| # A missing uvx download, a crash and a timeout all end here, because none of them prints a report. | |
| return None | |
| return findings |
🧰 Tools
🪛 ast-grep (0.45.3)
[error] 150-167: Command coming from incoming request
Context: subprocess.run(
[
*semgrep,
"--config",
str(REPO_ROOT / SEMGREP_RULES),
"--severity=WARNING",
"--severity=ERROR",
"--metrics=off",
"--quiet",
"--json",
".",
],
cwd=root,
env={**os.environ, "SEMGREP_ENABLE_VERSION_CHECK": "false"},
capture_output=True,
text=True,
timeout=_SEMGREP_TIMEOUT_SECONDS,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 03872caa-7a7b-4eb8-9ba2-ccfb532b6d3e
📒 Files selected for processing (3)
.agents/skills/running-ci-preflight/SKILL.mdtools/hogli-commands/hogli_commands/preflight_checks.pytools/hogli-commands/hogli_commands/tests/test_preflight_checks.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review.
350caf9 to
e740e67
Compare
|
Risk: No findings This PR adds four diff-reading checks to the Sentinel reviewed |
This branch has not been deployed
2 participants
Problem
frontend/snapshots.yml.Changes
hogli ci:preflightgains four checks. Each runs only when the diff touches its files.frontend-format--fixrepairssemgrep-devexsemgreponPATHsnapshot-baselinesmerge-queue-lanesemgrep-devexscans head copies in a temp directory, then merge-base copies of the flagged files. Semgrep's--baseline-commitrunsgit reset --hardon a clean checkout, so the check avoids it.semgrep-devexskips untilsemgrepis onPATH. chore(devex): add semgrep to the flox environment #110944 adds it to the flox environment, one version behind the CI pin, which is why the check advises and does not block.snapshot-baselinesdoes not block, because a branch can remove the entries of a story that an earlier PR deleted.DiffCheckgains aruncallable for checks that read the diff, andStatusmoved to the newpreflight_checks.py.How did you test this code?
Test rationale:
test_preflight_checks.pycovers the decision of each check: a grandfathered or moved finding must pass, a baseline removal passes only when its own story changed, and a lane warning needs a split that narrows.test_ci_preflight.pygains one case: only a failed check blocks a push, a crashing one skips, and a warn-only one stays out of the hook. No existing test covered these.tests/directory was ignored, which matches the CI directory scan.semgreponPATHthe check reported skipped. With a 1.172.0 binary onPATHit reported the seeded finding as an advisory.Release status
Automatic notifications
Docs update
The
running-ci-preflightskill describes the three new non-obvious checks.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Opus 5.5 (
claude-opus-5-5)/writing-tests,/writing-code-comments,/writing-skills,/writing-pr-descriptions./code-reviewat high effort ran twice and/simplifyonce, in place of the CodeRabbit CLI, at the assignee's standing preference. Fixed: explicit file targets bypassed semgrep's default ignores, renamed files lost their baseline, strict mode read uncommitted files, a crashing check blocked the push, the lane warning blamed files without proof, an incomplete scan read as clean, and the snapshot parser missed explicit-key entries. Not fixed: the semgrep scan directories and the oxfmt globs are copied from the CI workflow andpackage.json.errorsarray. Declined two Greptile findings. A change to a devex rule file is not scanned locally, because that needs the whole-tree scan CI runs. Strict mode formats working-tree copies, which is how every file-taking preflight check already works.