Skip to content

ci(codex): protect private-home auth preflight regression - #40

Merged
steipete merged 1 commit into
mainfrom
fix/codex-keychain-regression-ci-20260906
Sep 7, 2026
Merged

ci(codex): protect private-home auth preflight regression#40
steipete merged 1 commit into
mainfrom
fix/codex-keychain-regression-ci-20260906

Conversation

@steipete

@steipete steipete commented Sep 7, 2026

Copy link
Copy Markdown
Owner

Problem

A credential helper can succeed under the operator's HOME but fail with Keychain item-not-found under the reviewer's intentionally private HOME when it relies on implicit Keychain selection.

The owner-boundary repair and synthetic private-home diagnostic already landed in 362a562. However, CI did not run that regression suite, so its coverage did not protect subsequent changes.

Change

Run the existing Codex direct-route preflight suite in the smoke job with an empty environment, a fresh temporary HOME, closed stdin, and a 60-second timeout. The suite uses synthetic helpers and configuration; it needs no Keychain, account login, network access, or live credentials. Cleanup removes only the test-owned HOME.

No provider/model/tier selection, authentication wrapper, credential storage, reviewer HOME isolation, sandbox grants, or secret-scanning behavior changes.

Verification

  • Executed the exact added workflow step locally: passed.
  • actionlint .github/workflows/ci.yml: passed.
  • git diff --check: passed.
  • Negative control in a temporary copy: removing private-home validation caused the existing suite to fail specifically with HOME-dependent helper unexpectedly passed private-home check.
  • Actual macOS parent and private-home credential preflights passed without emitting credentials.

The synthetic CI suite protects the preflight behavior; it is not a live macOS Keychain integration test.

@clawsweeper

clawsweeper Bot commented Sep 7, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 7, 2026
@clawsweeper

clawsweeper Bot commented Sep 7, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed September 6, 2026, 10:33 PM ET / September 7, 2026, 02:33 UTC.

ClawSweeper review

What this changes

Add the existing Codex preflight regression suite to CI with an empty environment, temporary HOME, closed stdin, and timeout, and record the coverage in the changelog.

Merge readiness

Ready for maintainer review

This PR adds useful CI coverage that the pinned main workflow lacks. No actionable patch defect was found; the owner-authored PR also remains protected from automated closure.

Priority: P3
Reviewed head: 6d6275e35c5dc725233ac6471a826ae385052df8

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, conventional CI addition with relevant reported validation and no actionable defect found.
Proof confidence 🌊 off-meta tidepool Not applicable: This owner-authored CI-only PR is exempt from the external-contributor proof gate; its body reports the exact isolated suite run and a targeted negative control without claiming live Keychain integration coverage.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: This owner-authored CI-only PR is exempt from the external-contributor proof gate; its body reports the exact isolated suite run and a targeted negative control without claiming live Keychain integration coverage.
Evidence reviewed 7 items Verified introduced scope: The pinned base-to-head diff adds six workflow lines and two changelog lines. It does not change runtime authentication, provider settings, action references, permissions, or dependencies.
Current main still lacks this coverage: The pinned main smoke workflow runs npm and macOS-release helper tests but never invokes the Codex preflight suite. The existing runtime repair therefore does not make this CI addition redundant.
Synthetic regression and isolation boundary: The fully inspected suite creates its own configuration and shell helpers, invokes the repository's preflight with an explicit fixture config, rejects HOME-dependent delivery, verifies private-home cleanup, and checks that sentinel credentials never appear in output. This invocation requires neither Codex execution nor live Keychain access.
Findings None None.
Security None None.

How this fits together

This repository supplies shared agent skills and portable helpers. Its CI smoke job runs synthetic checks of the Codex configuration and credential-delivery preflight so regressions fail before changes land.

flowchart TD
  A[Push or pull request] --> B[CI smoke job]
  B --> C[Empty environment and temporary HOME]
  C --> D[Synthetic preflight suite]
  D --> E[Configuration and helper checks]
  E --> F[Pass or fail CI]
  C --> G[Remove temporary HOME]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Change scope 1 CI step added; 0 production or test-code changes The PR activates existing regression coverage without adding another implementation or test harness.

Technical review

Best possible solution:

Keep the existing synthetic preflight suite in the smoke job while preserving its separation from live credential and Keychain integration testing.

Do we have a high-confidence way to reproduce the issue?

Not applicable to this CI-only change: source inspection confirms the suite is absent from pinned main's workflow. The reviewer did not execute tests under the read-only review constraint.

Is this the best way to solve the issue?

Yes. Invoking the existing suite follows the adjacent isolated-test pattern and avoids duplicating regression logic.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against b511b24d36a0.

Labels

Label changes:

  • add P3: This is a bounded CI coverage improvement for an already-landed runtime repair.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This owner-authored CI-only PR is exempt from the external-contributor proof gate; its body reports the exact isolated suite run and a targeted negative control without claiming live Keychain integration coverage.

Label justifications:

  • P3: This is a bounded CI coverage improvement for an already-landed runtime repair.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This owner-authored CI-only PR is exempt from the external-contributor proof gate; its body reports the exact isolated suite run and a targeted negative control without claiming live Keychain integration coverage.

Evidence

What I checked:

  • Verified introduced scope: The pinned base-to-head diff adds six workflow lines and two changelog lines. It does not change runtime authentication, provider settings, action references, permissions, or dependencies. (.github/workflows/ci.yml:52, 6d6275e35c5d)
  • Current main still lacks this coverage: The pinned main smoke workflow runs npm and macOS-release helper tests but never invokes the Codex preflight suite. The existing runtime repair therefore does not make this CI addition redundant. (.github/workflows/ci.yml:44, b511b24d36a0)
  • Synthetic regression and isolation boundary: The fully inspected suite creates its own configuration and shell helpers, invokes the repository's preflight with an explicit fixture config, rejects HOME-dependent delivery, verifies private-home cleanup, and checks that sentinel credentials never appear in output. This invocation requires neither Codex execution nor live Keychain access. (skills/codex-huge-context/scripts/preflight.test.rb:201, 6d6275e35c5d)
  • Verification reported in captured PR body: The supplied PR body reports that the exact workflow step, actionlint, and whitespace checks passed locally; a temporary negative control failed at the HOME-dependent-helper assertion. It separately reports successful macOS preflights and explicitly distinguishes synthetic CI coverage from live Keychain integration. These are author-reported results, not reviewer-executed tests. (6d6275e35c5d)
  • Main-branch area history: Main-branch file history lists Peter Steinberger across private-home diagnostics, model coverage, split-provider validation, and context-limit maintenance. Exact line-introduction attribution could not be completed because required historical objects were unavailable and the remote could not resolve; no introducing-author claim is made. (skills/codex-huge-context/scripts/preflight.test.rb, 362a562bc2e0)
  • Repository policy: README identifies uppercase AGENTS.MD as the canonical instructions. That file was read fully; its changelog policy permits maintainer-added Unreleased entries in this repository. No applicable nested instructions or maintainer-notes directory were found. (AGENTS.MD, 6d6275e35c5d)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@steipete
steipete merged commit faf7b37 into main Sep 7, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant