Skip to content

chore(ci): gate EF migration index idempotency + worker guardrail docs - #433

Merged
thomasluizon merged 5 commits into
mainfrom
chore/ef-idempotency-ci-gate
Jul 24, 2026
Merged

chore(ci): gate EF migration index idempotency + worker guardrail docs#433
thomasluizon merged 5 commits into
mainfrom
chore/ef-idempotency-ci-gate

Conversation

@thomasluizon

@thomasluizon thomasluizon commented Jul 24, 2026

Copy link
Copy Markdown
Owner

Why

The EF raw-index idempotency rule (raw CREATE INDEX needs IF NOT EXISTS, raw DROP INDEX needs IF EXISTS) lived ONLY as an orbit-ui-mobile Claude PostToolUse hook. A codex worker, or a hook-bypassing commit, could merge a non-idempotent migration; the only catch was a failed Render deploy (EF re-applies at startup, a duplicate raw CREATE INDEX throws Postgres 42P07). This adds the missing CI twin so the rule holds for every tool.

What

  • .github/scripts/check_migration_idempotency.py: a faithful port of the checkEfMigrationRawIndex pure function. It scans only the balanced-paren body of each migrationBuilder.Sql(...) call, splits on ;, and flags a raw CREATE INDEX lacking IF NOT EXISTS or a raw DROP INDEX lacking IF EXISTS. The fluent migrationBuilder.CreateIndex(...)/DropIndex(...) API is not flagged (its PascalCase method names carry no whitespace, so they never match the raw-SQL patterns). Python is already the CI script language here (check_coverage.py, check_bench.py); no Node is needed.
  • .github/workflows/test.yml: wire it into the existing Guard Migrations job over the PR's changed Migrations/*.cs files only.
  • AGENTS.md: a "Guardrails you must not trip" subsection (no push to main, no --no-verify, no --no-gpg-sign, no worktree remove --force, EF index idempotency), stated plainly as enforced by CI/branch-protection/lefthook, not the Claude session hooks (which do not run under codex).
  • lefthook.yml: an engine-agnostic pre-push guard blocking push to main/master.

Verification

Fixture-for-fixture parity against the JS rule (identical BLOCK/pass verdicts):

fixture expected script JS rule
raw CREATE INDEX (no IF NOT EXISTS) FAIL exit 1 BLOCK
raw DROP INDEX (no IF EXISTS) FAIL exit 1 BLOCK
batched Sql(), good stmt masking a bad sibling FAIL exit 1 BLOCK
CREATE UNIQUE INDEX IF NOT EXISTS + DROP INDEX IF EXISTS PASS exit 0 pass
fluent CreateIndex(...)/DropIndex(...) PASS exit 0 pass
real AdoptPerformanceIndexes.cs migration PASS exit 0 pass

Pre-push guard tested across bare-push-on-main, explicit feature:main, and normal feature pushes.

Paired PR (consumer side, removes the false "shared with .opencode" comments and adds the git guardrail docs): thomasluizon/orbit-ui-mobile#585

The EF raw-index idempotency rule (raw CREATE INDEX needs IF NOT EXISTS, raw
DROP INDEX needs IF EXISTS) ran only as an orbit-ui-mobile Claude PostToolUse
hook, so a codex worker or a hook-bypassing commit could merge a non-idempotent
migration; the only catch was a failed Render deploy (Postgres 42P07).

- Add .github/scripts/check_migration_idempotency.py, a faithful port of the
  checkEfMigrationRawIndex pure function (balanced-paren Sql() bodies, split on
  ;, flags raw CREATE/DROP INDEX lacking IF [NOT] EXISTS; the fluent
  CreateIndex/DropIndex API is not flagged). Verified fixture-for-fixture
  against the JS rule: identical verdicts.
- Wire it into the Guard Migrations job over changed Migrations/*.cs files.
- AGENTS.md: add the worker guardrail rules (no push to main, no --no-verify,
  no --no-gpg-sign, no worktree remove --force, EF index idempotency) and state
  they are enforced by CI/branch-protection/lefthook, not the Claude hooks.
- lefthook.yml: add an engine-agnostic pre-push guard blocking push to
  main/master (backstop that fires under any tool, unlike the session hooks).

Paired with orbit-ui-mobile chore/worker-guardrails-agents-md.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #433 Review — chore(ci): gate EF migration index idempotency + worker guardrail docs

Scope: .github/scripts/check_migration_idempotency.py (new), .github/workflows/test.yml, AGENTS.md, lefthook.yml. CI/tooling + docs only, no application code. Reviewed against .claude/skills/pr-review/rubric.md.

Findings

[HIGH] Unquoted $changed expansion feeds python3 argv unsafely, and the required SonarCloud Quality Gate is currently failing on this PR
· dimension: 12. Security — Injection / Dependency & configuration
· location: .github/workflows/test.yml:126
· issue: python3 .github/scripts/check_migration_idempotency.py $changed expands $changed (a newline-separated git diff --name-only list) unquoted, so it undergoes shell word-splitting and glob expansion before reaching argv. SonarCloud has posted Quality Gate Failed on this PR (C Security Rating on New Code, required ≥ A) — this line is the only new security-relevant code path introduced in the diff, and the most plausible source of that finding. The SonarCloud API could not be queried from this sandbox to confirm the exact rule/line it mapped to, so treat that attribution as likely-but-unconfirmed; the failing required check itself is confirmed via the PR's own SonarCloud comment.
· risk: Practically, EF migration filenames are compiler-generated ({timestamp}_{Name}.cs) so real-world exploitability is low — worst case with a crafted filename is extra bogus paths reaching python3 (handled gracefully via the script's except (OSError, UnicodeDecodeError)), not code execution. But independent of exploitability, the required SonarCloud gate is failing right now, which blocks merge under this repo's branch-protection rules regardless of the underlying severity.
· fix: Avoid word-splitting on the file list, e.g. pipe through a null-delimited xargs:

git diff --name-only -z --diff-filter=d "origin/${{ github.base_ref }}...HEAD" -- ':(glob)**/Migrations/*.cs' \
  | xargs -0 --no-run-if-empty python3 .github/scripts/check_migration_idempotency.py

(adjust the "no migrations changed" short-circuit accordingly).
· reference: security category "Injection" (rubric dimension 12); SonarCloud Quality Gate (required check on this PR, confirmed FAILED via PR comment)

[MEDIUM] lefthook.yml pre-push guard blocks pushes that never touch the protected branch, contradicting its own message
· dimension: 1. Correctness
· location: lefthook.yml:14-20 (the branch=$(git rev-parse --abbrev-ref HEAD ...) / case "$branch" in main|master) blocked=1 ;; esac block)
· issue: The first loop in this hook already correctly blocks any push whose destination ref (read from stdin as remote_ref) is the protected branch — the actual invariant CLAUDE.md's branch-protection section cares about. The second check additionally sets blocked=1 whenever the currently checked-out local branch is named main/master, regardless of where the push is actually headed. Concretely: pushing a version tag while checked out on the protected branch, or re-pointing that local branch to a differently-named remote review/backup branch, trips this second check even though the remote ref being updated is not main/master at all.
· risk: A legitimate, non-protected push is rejected with a message claiming "direct push to main/master is forbidden" when the actual destination was never main/master. A developer chasing that false rejection is likely to reach for a hook-skip flag to route around it — which AGENTS.md's own new "Guardrails you must not trip" section (added in this same PR) explicitly forbids — rather than realizing the guard itself over-fired.
· fix: Drop the second (local-branch-name) check entirely; the remote_ref loop already captures "this push updates the protected branch" correctly and sufficiently.
· reference: orbit-api hard rule (git workflow / branch protection, CLAUDE.md "Git workflow")

Not verifiable in this CI job

  • Cross-repo parity (contract-aligner dimension): this PR touches no API contract/DTO, so N/A regardless; noting per instructions since orbit-ui-mobile isn't checked out here. The PR references a paired consumer PR (orbit-ui-mobile#585) not independently checked.

Non-findings (checked, no issue)

  • check_migration_idempotency.py is a faithful, self-contained port of the described JS rule; no eval/exec/shell=True, no path traversal, no content echoed beyond path:line messages. Regex set matches the stated scope (raw CREATE/DROP INDEX inside migrationBuilder.Sql(...), not the fluent API).
  • No unit test ships for the new Python script itself, but this matches the existing convention for the two sibling scripts in the same directory (check_bench.py, check_coverage.py are also untested) — not a new gap introduced by this PR.
  • AGENTS.md additions are accurate and consistent with CLAUDE.md's existing hard rules; no narration-comment or dead-code issues in any of the four changed files.
  • github.base_ref interpolation in the new workflow step is not attacker-controllable beyond the PR's own base-branch selection; no secrets or elevated permissions: are in scope for this job.

Recommendation

Request changes — one High finding tied to a currently-failing required check (SonarCloud Quality Gate), with a concrete fix. The Medium finding should be addressed but wouldn't independently block on its own.

thomasluizon and others added 2 commits July 24, 2026 14:39
Review feedback on PR #433 (SonarCloud "C Security Rating on New Code" gate +
a Medium correctness note):

- test.yml: stop word-splitting the changed-file list. The migration paths now
  flow null-delimited (git diff --name-only -z) into xargs -0, so no unquoted
  shell expansion or glob reaches python3 argv. Clears the SonarCloud new-code
  security finding while keeping identical behavior.
- lefthook.yml: drop the local-branch-name check from the pre-push guard. The
  stdin remote_ref loop already blocks any push whose destination is main/master
  (including a bare push while on main, since git passes refs/heads/main as the
  remote ref). The extra HEAD-name check over-fired on legitimate non-protected
  pushes made while checked out on main (a tag push, a backup-ref push) with a
  misleading "push to main" message.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SonarCloud flags `${{ github.base_ref }}` expanded directly inside a run: block
as a code-injection hotspot on new code (the C Security Rating that failed the
gate). Bind it once to an env var and reference "$BASE_REF" in the script, the
canonical remediation. No behavior change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@thomasluizon

Copy link
Copy Markdown
Owner Author

Addressed both review findings:

  • HIGH (SonarCloud C Security Rating on new code): two remediations. (1) The changed-file list now flows null-delimited (git diff --name-only -z) into xargs -0, so no unquoted $changed word-splitting or glob reaches python3 argv. (2) ${{ github.base_ref }} is no longer interpolated inside the run: script; it is bound to an env: BASE_REF and referenced as "$BASE_REF" (the canonical fix for the GitHub Actions code-injection hotspot, the most likely source of the new-code security rating).
  • MEDIUM (pre-push over-fire): dropped the local-branch-name check. The stdin remote_ref loop already blocks any push whose destination is main/master (including a bare push while on main, since git passes refs/heads/main as the remote ref), without falsely rejecting a tag or backup-ref push made while checked out on main.

Verified the guard and the xargs -0 pipeline locally across bare-push-on-main, feature:main, normal-feature, tag-push-on-main, and empty/good/bad fixture inputs.

thomasluizon and others added 2 commits July 24, 2026 14:43
The real SonarCloud "C Security Rating on New Code" finding was
pythonsecurity:S8707 at check_migration_idempotency.py: the script opened a
CLI-argument path without validating it stays inside the repo, so a crafted
argument could read a file outside the tree (path traversal).

Add the same `_confine` guard the sibling check_coverage.py already uses:
resolve each path with os.path.realpath and confirm it is within the working
directory before opening it; a path that escapes is reported and skipped.
Verified: relative migration paths pass, a `../` traversal path is refused.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: PR #433 (thomasluizon/orbit-api)

Scope: PR #433chore(ci): gate EF migration index idempotency + worker guardrail docs
Recommendation: APPROVE

Summary

Reviewed the diff since the last posted review (Claude review at commit 40e6aa0, 2026-07-24T17:34:22Z, CHANGES_REQUESTED). Both prior findings were addressed in the follow-up commit becd4e9 (fix(ci): address review on the EF idempotency gate), and each fix was verified independently rather than trusting the commit message.

  • [Previously HIGH] Unquoted $changed shell expansion → SonarCloud "C Security Rating" gate failure (.github/workflows/test.yml:126, old commit): fixed. The step now pipes git diff --name-only -z ... through xargs -0 --no-run-if-empty python3 .github/scripts/check_migration_idempotency.py — null-delimited, no shell word-splitting/glob expansion reaches argv. Confirmed against current HEAD:.github/workflows/test.yml. RESOLVED, no re-flag.
  • [Previously MEDIUM] lefthook.yml pre-push guard over-fired on non-main pushes made while checked out on main (lefthook.yml:14-20, old commit): fixed. The redundant branch=$(git rev-parse --abbrev-ref HEAD ...) / case "$branch" in main|master) block was removed; only the remote_ref stdin loop (the correct signal — the actual destination ref being pushed) remains. Confirmed against current HEAD:lefthook.yml. RESOLVED, no re-flag.

No new issues were introduced by the fix commit itself. The xargs -0 --no-run-if-empty invocation is well-formed GNU findutils syntax (present on ubuntu-latest runners); removing the early-exit "no migrations changed" branch is a behavior-neutral simplification (xargs simply skips invoking python3 on empty stdin, so the job still exits 0). The merge commit 2f00364 introduces no further diff beyond becd4e9.

Findings

Critical: None
High: None
Medium: None
Low / Info: None

Subagents

Agent Verdict
security-reviewer N/A — gate requires src/ code changed; this PR touches only .github/, AGENTS.md, lefthook.yml. Security dimension was still walked manually (as in the prior review) — no injection surface remains after the xargs -0 fix.
contract-aligner N/A — no DTO, Controller route, or packages/shared type changed.

Validation

Check Result
Build (dotnet) N/A — CI job invocation, Build/Unit Tests run as separate required checks
Tests (dotnet) N/A — CI job invocation, Build/Unit Tests run as separate required checks
SonarCloud Quality Gate Not independently re-verifiable here (no live SonarCloud query available in this sandbox). Note: the visible sonarqubecloud PR comment (posted 17:28:10Z) predates the fix commit becd4e9 (17:39:59Z) — it reflects the pre-fix state that triggered the original HIGH finding, not the current HEAD. CI's own SonarCloud re-run against the fixed commit is the authoritative signal.

Deferred — N/A dimensions & files not verdicted

  • Parity / i18n: frontend-only dimensions, owned by the orbit-ui-mobile-side review; no frontend files in this diff.
  • Contract drift + backward-compat: N/A — no packages/shared or DTO hunks in this diff.
  • Backend hard rules (ORBIT0001-0005 analyzers): N/A — no C# source files changed.
  • FEATURES.md parity: N/A — CI tooling change, no user-facing feature surface.
  • .github/scripts/check_migration_idempotency.py: unchanged since the prior review (not touched by the fix commit); prior review's "faithful port, no injection/traversal surface" verdict stands and is not re-litigated here.
  • AGENTS.md: unchanged since the prior review; prior verdict (accurate, consistent with CLAUDE.md) stands.

Every file changed in becd4e9 (.github/workflows/test.yml, lefthook.yml) received an explicit verdict above.

What's good

  • The fix commit applied exactly the recommended remediation for both findings, verified independently rather than taken on faith.
  • Fixture-for-fixture parity table in the PR description gives concrete before/after verification of the Python port against the JS rule it mirrors.
  • AGENTS.md's new "Guardrails you must not trip" section correctly scopes itself to CI/branch-protection/lefthook enforcement rather than claiming session-hook coverage that doesn't exist under codex exec.

Recommendation

APPROVE. No Critical/High findings survive; both prior findings are resolved with no regressions introduced by the fix.

@thomasluizon

Copy link
Copy Markdown
Owner Author

Addressed both findings (pushed).

HIGH / SonarCloud gate. I queried SonarCloud directly for PR 433: the actual failing issue was pythonsecurity:S8707 (MAJOR) at check_migration_idempotency.py:56, not the workflow line. The script opened a CLI-argument path without confining it, so a crafted path could escape the repo tree (traversal). Fixed by adding the same _confine guard the sibling check_coverage.py already uses (resolve with os.path.realpath, confirm it is within cwd before opening). Verified: relative migration paths pass, a ../ traversal path is refused.

I also kept the workflow hardening you flagged (it was a real robustness issue even though it was not the Sonar finding): the changed-file list now flows through a null-delimited git diff -z | xargs -0 --no-run-if-empty instead of an unquoted $changed, and github.base_ref is passed through an intermediate env: var rather than interpolated into the run script.

MEDIUM / lefthook over-fire. Dropped the local-branch-name fallback; the stdin remote_ref loop already blocks any push whose destination is main/master and no longer false-blocks a non-protected push (e.g. a tag) made while HEAD is on the protected branch.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Complete

Scope: PR #433 in thomasluizon/orbit-api — "chore(ci): gate EF migration index idempotency + worker guardrail docs"
Recommendation: APPROVE

Summary

This is a CI/tooling-only PR: a new Python idempotency-checker script for EF migrations, a wired-in "Guard Migrations" CI step, an AGENTS.md guardrails subsection, and a new lefthook pre-push guard blocking pushes to main/master. No src/ application code changed. A prior claude bot review (commit 40e6aa0) requested changes for a HIGH shell-injection-shaped finding (unquoted $changed word-splitting into python3 argv, tied to a failing SonarCloud Quality Gate) and a MEDIUM over-firing pre-push guard; both are fixed in the two follow-up commits (becd4e9, e3c8845) that are the delta reviewed here. Both fixes were re-verified directly against the cited lines and are correct and complete. One new Medium finding surfaced: the new lefthook pre-push hook isn't documented in root CLAUDE.md's "Git workflow" section, which this repo's own governance model treats as the single source of truth that AGENTS.md must defer to.

Findings

Critical

None.

High

None. Both findings from the prior review are resolved:

  • .github/workflows/test.yml now binds github.base_ref to env: BASE_REF and references it only as "$BASE_REF" inside the run: script (no more direct ${{ }} interpolation into the shell body), and the changed-file list flows NUL-delimited (git diff --name-only -z ... | xargs -0 --no-run-if-empty python3 ...) so no unquoted word-splitting/glob reaches python3 argv.
  • lefthook.yml's pre-push.protect-main no longer has the local-checked-out-branch-name check; it only inspects each remote_ref from stdin against refs/heads/main|refs/heads/master, so a tag push or a differently-named-branch push made while checked out on main no longer false-positives.

Medium

[MEDIUM] New pre-push guard isn't reflected in CLAUDE.md's Git workflow section, the repo's declared single source of truth
· dimension: 2 (documentation staleness) / repo governance rule
· location: orbit-api/CLAUDE.md:56-60 (Git workflow section) vs. orbit-api/AGENTS.md:24-41 (new "Guardrails you must not trip" section) and orbit-api/lefthook.yml:10-24
· issue: CLAUDE.md's "opencode compatibility" section states AGENTS.md "DEFERS to this CLAUDE.md for conventions; change behaviour here, never there." This PR adds real new enforced behavior — a lefthook pre-push hook that blocks pushes to main/master — and documents it in AGENTS.md, but CLAUDE.md's "Git workflow" section (lines 56-60) still only describes the pre-commit hook (dotnet format), with no mention of the new pre-push guard.
· risk: A future edit to the pre-push guard's behavior (or a reader relying on CLAUDE.md as the authoritative doc) won't see it reflected in the source-of-truth file, letting the two docs drift exactly the way the repo's own contract is meant to prevent.
· fix: Add 1-2 sentences to CLAUDE.md's Git workflow section alongside the existing pre-commit-hook paragraph, e.g.: "Pre-push hook: a git-level pre-push (lefthook) blocks any push whose destination ref is refs/heads/main or refs/heads/master, enforcing the no-direct-push rule client-side in addition to GitHub branch protection."
· reference: root CLAUDE.md "opencode compatibility" section (AGENTS.md defers to CLAUDE.md for conventions)

Low / Info

None posted (signal gate).

Subagents

Agent Verdict
security-reviewer N/A per gate (no src/ code changed) — manual security assessment performed instead given the CI-script/shell-injection surface; both previously-flagged issues confirmed fixed, no new issues found
contract-aligner N/A — no DTO, Controller route, or packages/shared/endpoints.ts surface touched

Validation

Check Result
Build (dotnet) N/A — skipped per instructions, CI runs it separately
Tests (dotnet) N/A — skipped per instructions, CI runs it separately

Deferred — N/A dimensions & files not verdicted

  • Dimension 3 (SOLID/clean-arch), 6 (type safety, C#/TS sense), 7 (console.log in app code), 8 (DESIGN.md/AI-slop), 9 (parity), 10 (i18n), 11 (contract drift), 13 (backend hard rules), 14 (FEATURES.md parity) — N/A, gated surfaces (src/, apps/*, DTOs, user-facing feature surface) not touched by this diff.
  • Cross-repo verification of the paired consumer PR (orbit-ui-mobile#585, referenced in the PR body) — not verifiable, sibling repo not checked out in this session.
  • /second-opinion cross-model step — not run; no Critical finding survived to trigger it, and it is UNAVAILABLE in this CI context regardless (no opencode).
  • All 4 changed files (check_migration_idempotency.py, test.yml, AGENTS.md, lefthook.yml) received an explicit verdict above — nothing silently skipped.

What's good

  • The two prior review findings were fixed precisely as recommended: BASE_REF via env: (canonical fix for the GitHub Actions script-injection hotspot) and NUL-delimited git diff -z | xargs -0 (eliminates the word-splitting hazard).
  • pre-push.protect-main is now minimal and correct — it enforces exactly the stated invariant (destination ref is the protected branch) with no over-fire.
  • check_migration_idempotency.py is a clean, self-contained, faithful port of the JS rule with sensible error handling (except (OSError, UnicodeDecodeError)) and no injection surface of its own (pure file read + regex, no eval/exec/shell=True).
  • AGENTS.md's new guardrails section is accurate and traceable to the actual enforcement mechanisms (CI job, branch protection, lefthook), correctly noting these hold even for engines where Claude Code session hooks don't run.

Recommendation

Merge-ready. The one Medium finding (CLAUDE.md doc staleness) is a good tracked follow-up but does not block — it's a documentation-consistency gap, not a functional or security issue.

@sonarqubecloud

Copy link
Copy Markdown

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: PR #433 (thomasluizon/orbit-api)

Scope: PR #433 — chore(ci): gate EF migration index idempotency + worker guardrail docs
Recommendation: APPROVE

Summary

Reviewed the diff since the last posted review (Claude APPROVE at commit becd4e9, 2026-07-24). Two commits landed since then, both fixing a SonarCloud finding (pythonsecurity:S8707, path traversal) that the earlier APPROVE review flagged as "not independently re-verifiable" at the time:

  • e3c8845 — .github/workflows/test.yml:117-127: moves github.base_ref out of direct ${{ }} interpolation inside the run: block and into env: BASE_REF, referenced as "$BASE_REF". This is the canonical GitHub Actions script-injection remediation.
  • 2d83bc5 — .github/scripts/check_migration_idempotency.py: adds a _confine(candidate, base) helper (resolve via os.path.realpath, reject anything outside cwd via resolved != base and not resolved.startswith(base + os.sep) — the separator-suffixed prefix check correctly avoids the classic /repo vs /repo-evil false-accept bug) and applies it to every CLI-argument path before open().

Both fixes are correct and were verified independently:

  • _confine is a byte-for-byte match of the existing pattern already used in the sibling .github/scripts/check_coverage.py:16-21, so it is consistent with established repo convention, not a novel/unreviewed idiom.
  • The env: indirection is scoped to only the one step (Fail on non-idempotent raw index SQL); it does not leak into other steps.
  • Guard Migrations CI job on this PR is green (SUCCESS), confirming the changed step still runs correctly end-to-end.

Findings

Critical: None
High: None
Medium: None
Low / Info:

  • .github/workflows/test.yml:105-113 — the sibling step immediately above (Fail on edits to applied migrations, pre-existing, unchanged by this PR) still interpolates ${{ github.base_ref }} directly into its run: block, the same pattern e3c8845 just remediated one step below. Not flagged as blocking: it is unchanged in this diff, and base_ref requires an attacker to already have push access to create a maliciously-named base branch in the target repo, so practical exploitability is low (same reasoning the prior review used). Worth a follow-up chore to apply the same env: treatment for consistency, since the two steps now visibly diverge in style within the same job.

Subagents

  • security-reviewer: N/A — gate requires src/ code changed; this round diff touches only .github/scripts/ and .github/workflows/. Security dimension walked manually — both fixes correctly close the SonarCloud S8707 (path traversal) and script-injection hotspots; no new surface introduced.
  • contract-aligner: N/A — no DTO, Controller route, or packages/shared type changed.

Validation

  • Build (dotnet): N/A — per caller instruction, CI runs Build/Unit Tests separately
  • Tests (dotnet): N/A — per caller instruction
  • Guard Migrations (live CI): PASS (observed via statusCheckRollup)
  • SonarCloud Analysis: IN_PROGRESS at time of review — not independently re-verifiable; the two commits reviewed here directly target the previously-reported S8707 finding with the established repo remediation pattern

Deferred — N/A dimensions and files not verdicted

  • AGENTS.md, lefthook.yml: unchanged since the prior APPROVE review (confirmed via git diff becd4e9 HEAD); prior verdicts stand, not re-litigated.
  • Parity / i18n: frontend-only dimensions; no frontend files in this diff.
  • Backend hard rules (ORBIT0001-0005 analyzers): N/A — no C# source under src/ changed.
  • FEATURES.md parity: N/A — CI tooling change only.
  • Cross-repo backward-compat (Phase 5): N/A — no contract surface touched; sibling orbit-ui-mobile#585 not independently checked (not verifiable in this CI, per instructions).

Every file touched since the last review (check_migration_idempotency.py, test.yml) received an explicit verdict above.

What is good

  • The _confine fix reuses the exact pattern already established in check_coverage.py rather than inventing a new one — good internal consistency.
  • The env: indirection fix is the textbook-correct remediation for GitHub Actions expression injection, applied precisely to the one step flagged.
  • Both fixes are minimal, scoped, and do not touch unrelated code.

Recommendation

APPROVE. No Critical/High findings survive on the delta since the last review. One non-blocking Low/Info consistency note (sibling step not updated to the same env: pattern) is worth a follow-up but does not affect this PR correctness or security posture.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: PR #433 (thomasluizon/orbit-api)

Scope: PR #433chore(ci): gate EF migration index idempotency + worker guardrail docs
Recommendation: APPROVE

Summary

Two prior bot reviews already ran on this PR: a CHANGES_REQUESTED at commit 40e6aa0 (HIGH: unquoted $changed shell expansion / failing SonarCloud gate; MEDIUM: lefthook.yml pre-push over-fire), and an APPROVE at commit 7a80583. However, the APPROVE review's text only discusses the becd4e9 fix commit — it never mentions three commits that actually landed before that SHA was recorded: e3c8845 (base_ref env-var externalization), 2d83bc5 (SonarCloud S8707 path-confinement fix), and 7a80583 itself (dropping an accidentally-committed .pyc). This pass focused on those three commits, which is the real "diff since last review."

Findings

Critical

None.

High

None.

Medium

  • .gitignore still has no Python bytecode entry, so this exact recurrence is unfixed at the root. 2d83bc5 accidentally committed .github/scripts/__pycache__/check_migration_idempotency.cpython-313.pyc, and 7a80583 had to delete it in a follow-up commit — but .gitignore has no __pycache__/ or *.pyc/*.py[cod] rule at all, despite .github/scripts/*.py now being a maintained part of this repo (check_coverage.py, check_bench.py, check_migration_idempotency.py). The symptom (stray artifact) was removed, but nothing prevents the next local python3 .github/scripts/check_*.py run from regenerating and re-committing a .pyc. Fix: add __pycache__/ (and/or *.py[cod]) to .gitignore. Non-blocking follow-up; does not change the recommendation.

Low / Info

  • e3c8845 (.github/workflows/test.yml:117-124): moves github.base_ref into an env: BASE_REF binding and quotes "$BASE_REF" everywhere it's used — the canonical GitHub Actions code-injection remediation, correctly and completely applied.
  • 2d83bc5 (.github/scripts/check_migration_idempotency.py:53-76): adds a _confine(candidate, base) guard (os.path.realpath + prefix check) before opening any CLI-argument path, mirroring the identical pattern already used in the sibling check_coverage.py:16-19. Traversal case is handled by printing to stderr and skipping, consistent with the sibling's convention; realistic attack surface is negligible since sys.argv only ever receives git diff --name-only -z output from the workflow itself.
  • 7a80583: confirmed the __pycache__/check_migration_idempotency.cpython-313.pyc binary is fully gone from the current tree (find .github -iname '*.pyc' → empty).
  • Guard Migrations CI check is SUCCESS on the live PR. SonarCloud Analysis was IN_PROGRESS at review time — could not confirm the final gate result, but the fix targets exactly the previously-cited rule (pythonsecurity:S8707) and reuses an already-accepted sibling pattern, so this is low-risk.

Subagents

Agent Verdict
security-reviewer N/A — gate requires src/ code changed; this PR touches only .github/, AGENTS.md, lefthook.yml. Manually walked the security-relevant hunks directly above; no injection/traversal issue remains.
contract-aligner N/A — no DTO, Controller route, or packages/shared type changed anywhere in this PR.

Validation

Check Result
Build (dotnet) N/A — skipped per instructions; covered by separate required CI checks
Tests (dotnet) N/A — skipped per instructions; covered by separate required CI checks
Guard Migrations (live CI) SUCCESS
SonarCloud Analysis (live CI) IN_PROGRESS at review time — not independently confirmable

Deferred — N/A dimensions

  • Parity / i18n: frontend-only dimensions, no frontend files in this diff.
  • Backward-compat guard (Phase 5): N/A — no packages/shared or DTO hunks anywhere in this PR.
  • Backend hard rules (ORBIT0001-0005 analyzers): N/A — no C# source changed by any of the three reviewed commits.
  • FEATURES.md parity: N/A — CI-tooling-only change.
  • Cross-repo dimensions (contract-aligner, packages/shared side of backward-compat guard): not verifiable in this job — orbit-ui-mobile isn't checked out here; not guessed.
  • AGENTS.md / final lefthook.yml state: unchanged since the second (APPROVE) review, which already verdicted them — not re-litigated.

What's good

  • Both remediations (env: binding for base_ref, _confine() for path traversal) are textbook-correct fixes for their respective Sonar findings and reuse an already-established in-repo pattern rather than inventing a new one.
  • The transient .pyc mistake was caught and cleaned up within the same PR rather than shipped.

Recommendation

APPROVE. No Critical/High findings survive in the diff since the last review. One non-blocking Medium hygiene item (.gitignore missing __pycache__/*.pyc) is worth a quick follow-up, but it doesn't change the recommendation.

@thomasluizon
thomasluizon merged commit bcbbfeb into main Jul 24, 2026
21 checks passed
@thomasluizon
thomasluizon deleted the chore/ef-idempotency-ci-gate branch July 24, 2026 17:51
thomasluizon added a commit to thomasluizon/orbit-ui-mobile that referenced this pull request Jul 24, 2026
Drop the local-branch-name fallback added in #585 so the two repos' pre-push
guards behave identically. The stdin remote_ref loop already blocks any push
whose destination is main/master; the branch-name check additionally false-blocked
a non-protected push (e.g. a tag) made while HEAD is on the protected branch,
with a misleading "pushing to main/master" message. Matches the fix applied in
thomasluizon/orbit-api#433.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thomasluizon added a commit that referenced this pull request Jul 24, 2026
…ting every push (#434)

* chore(skills): the Phase 6 skill-quality sweep (orbit-ui-mobile REBUILD.md 10.2)

Swept this repo's two skills against the writing-great-skills rubric.
Applied only changes that improve predictability, reduce context load, or
remove duplication and sediment. No trigger, completion criterion, phase
number, or output contract was changed.

Both skills carry a hand-maintained lockstep contract with their
orbit-ui-mobile twins, so only api-only text was edited, plus one dead
pointer: the `.claude/plans/completed/` path has never existed in this
repo and is gone from orbit-ui-mobile.

Cross-repo drift found and reported for a paired follow-up rather than
fixed one-sidedly, since each fix is a behaviour change:

- The ui copy's blocking fan-out contract is missing here, so a stranded
  api review posts nothing.
- `.github/workflows/claude-review.yml` cites stale phase numbers: it says
  skip "Phase 6 (dotnet build / test)" and "Phase 7 posting", but validate
  is Phase 7 and posting is Phase 8. Followed literally, CI skips the
  adversarial verification pass and runs the validation it was told to skip.
- `rubric.md` dimension 8 is stale against the post-#539 DESIGN.md in BOTH
  repos: it still sanctions the gradient header and asks for violet-glow
  character, both deleted by the design freeze.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018M2vH4pDvNnxUPecWWvwBc

* fix(lefthook): the pre-push guard rejected every push, not just main

The `protect-main` guard added in #433 used a multi-line shell script in
lefthook's inline `run:` field. On Windows that script does not survive
lefthook's shell invocation: it dies with "-c: line 9: syntax error:
unexpected end of file" before reading a single ref. lefthook treats the
non-zero exit as a hook failure, so the guard blocked EVERY push.

Reproduced deterministically with
`printf 'refs/heads/chore/x abc refs/heads/chore/x def\n' | lefthook run pre-push`,
which is a plain feature branch and must pass.

Moved the logic into `tools/check-push-target.mjs`, invoked as a single
command. `tools/*.mjs` is already pinned to LF in `.gitattributes`, so the
script is also immune to line-ending drift on a fresh Windows clone.

Verified both directions after the change:
- feature branch ref: protect-main passes
- refs/heads/main:    BLOCKED, exit status 1

Note for orbit-ui-mobile: it carries the identical inline guard from #589
and is equally broken, but the failure is currently invisible there
because `core.hooksPath` is redirected and no pre-push hook is installed,
so the guard never runs. That repo needs the same fix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018M2vH4pDvNnxUPecWWvwBc

* fix(agents): security-reviewer never loaded, so /pr-review had no security pass

Its frontmatter `description` is an unquoted YAML scalar containing ": "
(in "security issues: missing [Authorize]"), which breaks the parse. The
agent was silently dropped: all 10 sibling agents in orbit-ui-mobile
loaded while this one was absent from the session's agent list. Ruled out
the alternatives first: no duplicate `name` in the directory, no
agent-disabling key in any settings scope.

`/pr-review` names security-reviewer as one of the subagents it
orchestrates, so its security dimension has been running as a no-op.

Fixed by moving the description to a folded block scalar, the same repair
applied to five skills in the paired orbit-ui-mobile PR. Verified live:
the agent appeared in the available-agent list immediately after.

Third instance of this same YAML bug found today (5 skills, 2 agents), so
it wants a CI gate rather than another manual fix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018M2vH4pDvNnxUPecWWvwBc

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant