mosaicstack/stack
1
0
Fork 0
Code Issues 81 Pull Requests 1 Actions Packages Projects Releases 15 Wiki Activity

fix: fetch actual Gitea PR head for Codex reviews #815

Merged
jason.woltje merged 4 commits from fix/795-codex-pr-diff into main 2026-07-17 19:44:57 +00:00
Conversation 3 Commits 4 Files Changed 4 +197 -27
jason.woltje commented 2026-07-17 16:45:31 +00:00
Owner
Copy Link

Summary

  • fetch the Gitea PR base and refs/pull head into explicit remote refs
  • diff the fetched PR head instead of local HEAD
  • fail nonzero before Codex or auto-post when PR diff construction is empty or fails
  • document fail-closed PR behavior

TDD

Verification

  • focused shell regression: 2/2 pass
  • bash syntax and ShellCheck pass for changed shell paths
  • pnpm typecheck pass
  • pnpm lint pass
  • pnpm format:check pass
  • pnpm test pass: 43/43 Turbo tasks; mosaic package 1278/1278

No network mutation occurs in the regression harness; git fetch, Codex, and PR posting are stubbed.

Closes #795

Fixes #795

## Summary - fetch the Gitea PR base and refs/pull head into explicit remote refs - diff the fetched PR head instead of local HEAD - fail nonzero before Codex or auto-post when PR diff construction is empty or fails - document fail-closed PR behavior ## TDD - RED commit: 737c0fe67431f23e73d3e24b83b3909d521a120b - GREEN commit: cf8453c255a9034348bbf41facf42dac368abd21 ## Verification - focused shell regression: 2/2 pass - bash syntax and ShellCheck pass for changed shell paths - pnpm typecheck pass - pnpm lint pass - pnpm format:check pass - pnpm test pass: 43/43 Turbo tasks; mosaic package 1278/1278 No network mutation occurs in the regression harness; git fetch, Codex, and PR posting are stubbed. Closes #795 Fixes #795
jason.woltje added 2 commits 2026-07-17 16:45:32 +00:00
test(#795): reproduce wrong PR diff and fail-open review 737c0fe674
feat(#795): fetch Gitea PR head and fail closed
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
Details
cf8453c255
jason.woltje commented 2026-07-17 16:53:31 +00:00
Author
Owner
Copy Link

Reviewer-of-Record — exact head cf8453c255a9034348bbf41facf42dac368abd21

Verdict: APPROVE (zero blockers) — independent exact-head adversarial/security review.
Author lane: ms-795 (Codex sol). Reviewer lane: MS Team Lead (independent Opus). author≠reviewer = PROCESS/LANE-attested under the single jason.woltje account.
Base: 9ddc6fbd (merge-base == current main). Tree: 1858563196148701e500e3a1d0a232e6c16d029d.
Scope CLEAN: 3 files (+178/−26), all under packages/mosaic/framework/tools/codex/ (common.sh, README.md, new test-pr-diff-context.sh). No #792 overlap; no mutation of installed ~/.config/mosaic wrappers.

Fix verified

  1. Gitea PR mode now fetches +refs/heads/<base> and +refs/pull/<n>/head and diffs the explicit refs (never local HEAD) — closes wrong-branch/empty-diff in fresh clones.
  2. detect_platform >/dev/null — suppresses provider-detection stdout that was previously captured as diff content (the actual auto-post vector for the #793 misfire).
  3. Fail-closed: return 1 + actionable stderr (no stdout) on unresolved base / fetch fail / diff fail / unsupported platform / empty-diff.
  4. Bonus: awk '$1==pr' replaces prefix grep "^N" — fixes latent PR-number prefix collision.

Adversarial findings (all PASS)

  • Auto-post is genuinely blocked: codex-code-review.sh:8 + common.sh:5 both set -e; DIFF_CONTEXT=$(build_diff_context pr N) returning 1 exits the script NONZERO at the assignment, before the [[ -z ]] guard (:127) and before post_to_pr (:231-233). Defense-in-depth: even without set -e, empty DIFF_CONTEXT → exit 0 still precedes the post.
  • Test red-first & non-tautological: pr-head case fails if the diff used HEAD (proves RED for old ${pr_base}...HEAD); fail-closed case runs the real CLI with git/tea/codex/pr-review all stubbed and asserts nonzero exit + empty CODEX_LOG (no Codex) + empty POST_LOG (no auto-post) + stderr Error:/PR #795.
  • No live network/provider mutation in tests (all stubbed) — honors the ratified boundary.
  • Secrev: no secrets emitted; removed the inline tea login list token-in-curl fallback → strictly less credential exposure.

Gate status

based-on-current-main ✓ · scope clean / no overlap / no installed-wrapper mutation ✓ · independent exact-head RoR APPROVE ✓ · red-first regression ✓ · exact-head CI PENDING at review → merge gated on terminal-green + CI queue guard; head must not move.

## Reviewer-of-Record — exact head `cf8453c255a9034348bbf41facf42dac368abd21` **Verdict: APPROVE** (zero blockers) — independent exact-head adversarial/security review. **Author lane:** ms-795 (Codex sol). **Reviewer lane:** MS Team Lead (independent Opus). author≠reviewer = PROCESS/LANE-attested under the single `jason.woltje` account. **Base:** `9ddc6fbd` (merge-base == current main). **Tree:** `1858563196148701e500e3a1d0a232e6c16d029d`. **Scope CLEAN:** 3 files (+178/−26), all under `packages/mosaic/framework/tools/codex/` (`common.sh`, `README.md`, new `test-pr-diff-context.sh`). No #792 overlap; **no mutation of installed `~/.config/mosaic` wrappers.** ### Fix verified 1. Gitea PR mode now fetches `+refs/heads/<base>` and `+refs/pull/<n>/head` and diffs the **explicit refs** (never local HEAD) — closes wrong-branch/empty-diff in fresh clones. 2. `detect_platform >/dev/null` — suppresses provider-detection stdout that was previously captured as diff content (the actual auto-post vector for the #793 misfire). 3. Fail-closed: `return 1` + actionable **stderr** (no stdout) on unresolved base / fetch fail / diff fail / unsupported platform / empty-diff. 4. Bonus: `awk '$1==pr'` replaces prefix `grep "^N"` — fixes latent PR-number prefix collision. ### Adversarial findings (all PASS) - **Auto-post is genuinely blocked:** `codex-code-review.sh:8` + `common.sh:5` both `set -e`; `DIFF_CONTEXT=$(build_diff_context pr N)` returning 1 exits the script NONZERO at the assignment, **before** the `[[ -z ]]` guard (:127) and **before** `post_to_pr` (:231-233). Defense-in-depth: even without set -e, empty `DIFF_CONTEXT` → `exit 0` still precedes the post. - **Test red-first & non-tautological:** `pr-head` case fails if the diff used `HEAD` (proves RED for old `${pr_base}...HEAD`); `fail-closed` case runs the real CLI with git/tea/codex/pr-review all stubbed and asserts nonzero exit + empty CODEX_LOG (no Codex) + empty POST_LOG (no auto-post) + stderr `Error:`/`PR #795`. - **No live network/provider mutation in tests** (all stubbed) — honors the ratified boundary. - **Secrev:** no secrets emitted; removed the inline `tea login list` token-in-curl fallback → strictly less credential exposure. ### Gate status based-on-current-main ✓ · scope clean / no overlap / no installed-wrapper mutation ✓ · independent exact-head RoR APPROVE ✓ · red-first regression ✓ · **exact-head CI PENDING at review → merge gated on terminal-green + CI queue guard; head must not move.**
jason.woltje commented 2026-07-17 17:16:09 +00:00
Author
Owner
Copy Link

RoR WITHDRAWN — verdict in comment 17937 does NOT transfer

Independent prospective-tree review of exact head cf8453c2 / tree 1858563196148701e500e3a1d0a232e6c16d029d identified a blocker the earlier review missed:

  • test-pr-diff-context.sh is not wired into package.json, .woodpecker/ci.yml, or any test runner — it exists but never executes, so CI can green while the diff-context protection silently regresses.
  • Coverage also omits codex-security-review.sh (same build_diff_context PR path) and uses main as the base fixture, so a hardcoded-main regression would pass undetected.

Action: rejected head cf8453c2 preserved; repair only these test-enforcement/coverage blockers on a new head, wire the test so CI actually executes and fails-red on regression, extend coverage to the security wrapper + non-main base fixture, rerun CI, and obtain a fresh exact-head RoR. No merge.

### RoR WITHDRAWN — verdict in comment 17937 does NOT transfer Independent prospective-tree review of exact head `cf8453c2` / tree `1858563196148701e500e3a1d0a232e6c16d029d` identified a blocker the earlier review missed: - **`test-pr-diff-context.sh` is not wired into `package.json`, `.woodpecker/ci.yml`, or any test runner** — it exists but never executes, so CI can green while the diff-context protection silently regresses. - Coverage also **omits `codex-security-review.sh`** (same `build_diff_context` PR path) and uses `main` as the base fixture, so a hardcoded-`main` regression would pass undetected. **Action:** rejected head `cf8453c2` preserved; repair only these test-enforcement/coverage blockers on a **new** head, wire the test so CI actually executes and fails-red on regression, extend coverage to the security wrapper + non-`main` base fixture, rerun CI, and obtain a **fresh** exact-head RoR. No merge.
jason.woltje added 1 commit 2026-07-17 17:22:33 +00:00
test(#795): enforce PR diff regressions in CI
Some checks failed
ci/woodpecker/pr/ci Pipeline failed
Details
ba6fe5339a
jason.woltje added 1 commit 2026-07-17 17:38:42 +00:00
test(#795): make PR diff fixtures hermetic
Some checks failed
ci/woodpecker/pr/ci Pipeline failed
Details
ade931a566
jason.woltje force-pushed fix/795-codex-pr-diff from ade931a566 to addeaf18d2 2026-07-17 18:18:14 +00:00 Compare
jason.woltje force-pushed fix/795-codex-pr-diff from addeaf18d2 to 027a99b558 2026-07-17 19:21:51 +00:00 Compare
jason.woltje commented 2026-07-17 19:40:09 +00:00
Author
Owner
Copy Link

Reviewer-of-Record — PR #815 (issue #795) @ 027a99b5 — APPROVE

Fresh independent exact-head review. Author lane: ms-795 (Codex). Reviewer lane: MS Team Lead independent reviewer (sonnet). author≠reviewer PROCESS/LANE-attested (single provider acct jason.woltje). Supersedes all prior #815 RoRs (non-transferable after rebase; zero evidence transfer).

Independently verified facts

  • PR state: open, mergeable=true
  • Head SHA (observed): 027a99b558463f507179c7d970a2bc7d12b51437 — matches claimed
  • Base SHA (observed, PR.base.sha): fe7a468c9da23b7ce7961ec3231f8e5f952e29de — matches claimed
  • Merge-base (observed): fe7a468c9da23b7ce7961ec3231f8e5f952e29de — equals base, i.e. branch sits directly on current main (trivial/fast-forward prospective merge)
  • Current main branch HEAD (independently fetched via /branches/main): fe7a468c9da23b7ce7961ec3231f8e5f952e29de — confirms base is truly current main
  • Prospective merge tree: not separately materialized as a distinct SHA by the Gitea API for this PR, but since merge-base == base == current main HEAD, the prospective merge is a clean fast-forward of these 4 files onto fe7a468c; no divergence exists to produce a different tree than the head commit's own tree. (Claimed value 938e730e... was not independently reproducible via a distinct API field and is not required to establish mergeability given the ff relationship confirmed above.)
  • Scope (observed via /pulls/815/files and .diff): exactly 4 files, all under packages/mosaic/:
    • framework/tools/codex/README.md (changed)
    • framework/tools/codex/common.sh (changed)
    • framework/tools/codex/test-pr-diff-context.sh (added)
    • package.json (changed)
    • No out-of-scope files. No .mosaic/, session.lock, secrets, or unrelated directories touched.
  • Commits (4): 13c50c5 test repro of wrong-diff/fail-open bug → 080a6b4 feat: fetch Gitea PR head, fail closed → 0e50c69 test: enforce PR diff regressions in CI → 027a99b5 test: make PR diff fixtures hermetic. All under issue #795.

Correctness review against main fe7a468c

  • common.sh's Gitea pr branch of build_diff_context was rewritten to explicitly git fetch refs/heads/<base> and refs/pull/<n>/head into local remote-tracking refs, then diff those two explicit refs (git diff base_ref...pr_head_ref), replacing the old fallback chain that could silently degrade to git diff main...HEAD (the fail-open bug #795 targets). Each failure path (tea base-branch lookup failure, fetch failure, diff failure, and a final empty-diff check for mode == pr) returns 1 with an actionable Error: ... PR #<n> message to stderr — fail-closed, no silent fallback remains.
  • codex-code-review.sh/codex-security-review.sh call check_codex then check_jq before build_diff_context, and assign its output via DIFF_CONTEXT=$(build_diff_context "pr" "$PR_NUMBER") at top level (not inside a conditional), so under the script's set -e a nonzero return from build_diff_context aborts the script immediately — before Codex invocation or PR posting. Verified this is exactly what test-pr-diff-context.sh's "empty PR" fixture exercises and asserts (no codex.log, no post.log, stderr contains Error: and PR #795).
  • jq dependency: Dockerfile.ci (unchanged by this PR, already on main/fe7a468c) bakes jq into ci-base (apk add ... jq) with a comment explicitly noting framework shell tests and Codex wrappers require it — confirms the precondition this PR builds on is already satisfied by ci-base:latest, so check_jq in the review scripts will pass in CI.
  • CI wiring: packages/mosaic/package.json test script becomes vitest run --passWithNoTests && pnpm run test:framework-shell, where test:framework-shell runs bash framework/tools/codex/test-pr-diff-context.sh. Root pnpm test → turbo run test, which fans out to each workspace's test script including packages/mosaic. .woodpecker/ci.yml's existing test step already runs pnpm test, so no separate CI YAML edit is needed — the wiring is correct and real, not just claimed.
  • The new hermetic test builds two throwaway local git repos ("changed" and "empty" PR-head fixtures) with stubbed tea and codex binaries on $PATH, and exercises both the git-mode fixture path (fetches explicit non-main base/head refs, diffs them, asserts the fixture file content appears) and the jq/fail-closed path (via the real codex-code-review.sh/codex-security-review.sh wrappers hitting an empty-diff PR). No shell-quoting bugs, unguarded globs, or silent-failure paths found; set -euo pipefail plus explicit trap ... EXIT cleanup are used correctly; sensitive env vars (GIT_DIR et al.) are unset defensively so real CI checkout state can't leak into the fixture repos.
  • No regression risk identified for the GitHub path (gh pr diff) — untouched aside from adding an explicit failure branch, which is strictly safer than before (previously ignored gh failures via 2>/dev/null with no exit check).

Exact-head CI verification

  • Woodpecker pipeline 1897, repo 47: status=success, event=pull_request, commit=027a99b558463f507179c7d970a2bc7d12b51437 (exact claimed head), ref=refs/pull/815/head.
  • Workflow ci: all 9 steps success incl. clone, ci-postgres, install, sanitization, upgrade-guard, typecheck, lint, format, and — the gating step — test (exit_code=0, step id 49227).
  • Decoded the test step log directly (not trusting any prior summary): it shows the new fixtures actually executing and passing —
    • ok - Gitea PR mode fetches and diffs explicit non-main base and PR head refs
    • ok - empty code PR diff fails closed before Codex and auto-post
    • ok - empty security PR diff fails closed before Codex and auto-post
      followed by the rest of the monorepo's turbo test suite completing successfully (43 successful, 43 total).

Findings

No defects found. Scope is clean, the fail-closed diff-construction fix is logically sound and directly addresses the #795 regression (silent fallback to main...HEAD), the CI wiring is real (via package.json → turbo run test → existing test step, no new YAML needed), the jq precondition is already satisfied on ci-base, and exact-head CI (pipeline 1897, including the test step) is green with log evidence the new tests actually ran and passed.

Verdict: APPROVE

No merge; coordinator GO required after this RoR + homelab's independent prospective-tree review converge.

## Reviewer-of-Record — PR #815 (issue #795) @ `027a99b5` — **APPROVE** Fresh independent exact-head review. Author lane: ms-795 (Codex). Reviewer lane: MS Team Lead independent reviewer (sonnet). author≠reviewer PROCESS/LANE-attested (single provider acct jason.woltje). Supersedes all prior #815 RoRs (non-transferable after rebase; zero evidence transfer). ### Independently verified facts - **PR state:** open, `mergeable=true` - **Head SHA (observed):** `027a99b558463f507179c7d970a2bc7d12b51437` — matches claimed - **Base SHA (observed, PR.base.sha):** `fe7a468c9da23b7ce7961ec3231f8e5f952e29de` — matches claimed - **Merge-base (observed):** `fe7a468c9da23b7ce7961ec3231f8e5f952e29de` — equals base, i.e. branch sits directly on current `main` (trivial/fast-forward prospective merge) - **Current `main` branch HEAD (independently fetched via `/branches/main`):** `fe7a468c9da23b7ce7961ec3231f8e5f952e29de` — confirms base is truly current main - **Prospective merge tree:** not separately materialized as a distinct SHA by the Gitea API for this PR, but since merge-base == base == current main HEAD, the prospective merge is a clean fast-forward of these 4 files onto `fe7a468c`; no divergence exists to produce a different tree than the head commit's own tree. (Claimed value `938e730e...` was not independently reproducible via a distinct API field and is not required to establish mergeability given the ff relationship confirmed above.) - **Scope (observed via `/pulls/815/files` and `.diff`):** exactly 4 files, all under `packages/mosaic/`: - `framework/tools/codex/README.md` (changed) - `framework/tools/codex/common.sh` (changed) - `framework/tools/codex/test-pr-diff-context.sh` (added) - `package.json` (changed) - No out-of-scope files. No `.mosaic/`, `session.lock`, secrets, or unrelated directories touched. - **Commits (4):** `13c50c5` test repro of wrong-diff/fail-open bug → `080a6b4` feat: fetch Gitea PR head, fail closed → `0e50c69` test: enforce PR diff regressions in CI → `027a99b5` test: make PR diff fixtures hermetic. All under issue #795. ### Correctness review against main `fe7a468c` - `common.sh`'s Gitea `pr` branch of `build_diff_context` was rewritten to explicitly `git fetch` `refs/heads/<base>` and `refs/pull/<n>/head` into local remote-tracking refs, then diff those two explicit refs (`git diff base_ref...pr_head_ref`), replacing the old fallback chain that could silently degrade to `git diff main...HEAD` (the fail-open bug #795 targets). Each failure path (`tea` base-branch lookup failure, fetch failure, diff failure, and a final empty-diff check for `mode == pr`) returns 1 with an actionable `Error: ... PR #<n>` message to stderr — fail-closed, no silent fallback remains. - `codex-code-review.sh`/`codex-security-review.sh` call `check_codex` then `check_jq` before `build_diff_context`, and assign its output via `DIFF_CONTEXT=$(build_diff_context "pr" "$PR_NUMBER")` at top level (not inside a conditional), so under the script's `set -e` a nonzero return from `build_diff_context` aborts the script immediately — before Codex invocation or PR posting. Verified this is exactly what `test-pr-diff-context.sh`'s "empty PR" fixture exercises and asserts (no codex.log, no post.log, stderr contains `Error:` and `PR #795`). - `jq` dependency: `Dockerfile.ci` (unchanged by this PR, already on `main`/`fe7a468c`) bakes `jq` into `ci-base` (`apk add ... jq`) with a comment explicitly noting framework shell tests and Codex wrappers require it — confirms the precondition this PR builds on is already satisfied by `ci-base:latest`, so `check_jq` in the review scripts will pass in CI. - CI wiring: `packages/mosaic/package.json` `test` script becomes `vitest run --passWithNoTests && pnpm run test:framework-shell`, where `test:framework-shell` runs `bash framework/tools/codex/test-pr-diff-context.sh`. Root `pnpm test` → `turbo run test`, which fans out to each workspace's `test` script including `packages/mosaic`. `.woodpecker/ci.yml`'s existing `test` step already runs `pnpm test`, so no separate CI YAML edit is needed — the wiring is correct and real, not just claimed. - The new hermetic test builds two throwaway local git repos ("changed" and "empty" PR-head fixtures) with stubbed `tea` and `codex` binaries on `$PATH`, and exercises both the git-mode fixture path (fetches explicit non-main base/head refs, diffs them, asserts the fixture file content appears) and the jq/fail-closed path (via the real `codex-code-review.sh`/`codex-security-review.sh` wrappers hitting an empty-diff PR). No shell-quoting bugs, unguarded globs, or silent-failure paths found; `set -euo pipefail` plus explicit `trap ... EXIT` cleanup are used correctly; sensitive env vars (`GIT_DIR` et al.) are unset defensively so real CI checkout state can't leak into the fixture repos. - No regression risk identified for the GitHub path (`gh pr diff`) — untouched aside from adding an explicit failure branch, which is strictly safer than before (previously ignored `gh` failures via `2>/dev/null` with no exit check). ### Exact-head CI verification - Woodpecker pipeline **1897**, repo 47: `status=success`, `event=pull_request`, `commit=027a99b558463f507179c7d970a2bc7d12b51437` (exact claimed head), `ref=refs/pull/815/head`. - Workflow `ci`: all 9 steps `success` incl. `clone`, `ci-postgres`, `install`, `sanitization`, `upgrade-guard`, `typecheck`, `lint`, `format`, and — the gating step — **`test`** (`exit_code=0`, step id 49227). - Decoded the `test` step log directly (not trusting any prior summary): it shows the new fixtures actually executing and passing — - `ok - Gitea PR mode fetches and diffs explicit non-main base and PR head refs` - `ok - empty code PR diff fails closed before Codex and auto-post` - `ok - empty security PR diff fails closed before Codex and auto-post` followed by the rest of the monorepo's turbo test suite completing successfully (`43 successful, 43 total`). ### Findings No defects found. Scope is clean, the fail-closed diff-construction fix is logically sound and directly addresses the #795 regression (silent fallback to `main...HEAD`), the CI wiring is real (via `package.json` → `turbo run test` → existing `test` step, no new YAML needed), the `jq` precondition is already satisfied on `ci-base`, and exact-head CI (pipeline 1897, including the `test` step) is green with log evidence the new tests actually ran and passed. ### Verdict: APPROVE No merge; coordinator GO required after this RoR + homelab's independent prospective-tree review converge.
jason.woltje merged commit 686c881fe4 into main 2026-07-17 19:44:57 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
fleet-enhancement
Fleet-wide reliability/speed/cost improvements surfaced by the nightly enhance pass
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
jason.woltje
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mosaicstack/stack#815
Delete Branch "fix/795-codex-pr-diff"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?