Repository navigation
fix(release): pipeline correctness for Windows packaging, checksums, publication ordering, and process identity - #5388
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe pull request updates release packaging and publication ordering, expands service lifecycle path coverage, disables desktop proxy use for loopback requests, and recognizes Windows ChangesRelease pipeline and service lifecycle
Desktop direct transport
Windows command detection
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant package-standalone
participant package-desktop
participant publish
participant attach-release
package-standalone->>publish: complete standalone packaging
package-desktop->>publish: complete desktop packaging
publish->>attach-release: complete publication
attach-release->>attach-release: verify dist/release checksums
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6903852fb6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| || normalized.includes("@bitkyc08/opencodex") | ||
| || /@bitkyc08\/\.opencodex-/.test(normalized) | ||
| || /(?:^|[\s/"'])(?:ocx|opencodex)(?:\.cmd)?(?:$|[\s"'])/.test(normalized); | ||
| || /(?:^|[\s/"'])(?:ocx|opencodex)(?:\.cmd|\.exe)?(?:$|[\s"'])/.test(normalized); |
There was a problem hiding this comment.
This changes the command-line identity contract in src/config/process-state.ts, but the commit does not update either structure/runtime.md or structure/config.md, which structure/INDEX.md maps as owners of src/config/. Document the newly accepted Windows .exe process shape and its identity boundary in both mapped documents so the architecture SSoT remains synchronized with runtime behavior.
AGENTS.md reference: src/AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
…ifiable checksums The Windows desktop packaging lane ran a bash script under the runner default shell: Rename release assets uses backslash continuations and "$VAR" expansion with no shell: bash, and the workflow sets no defaults.run.shell, so a Windows runner reads it as PowerShell. The standalone archive step also wrote checksums naming a dist/ path, while attach-release verifies them with shasum -c from inside dist/release, where no dist/ directory exists. The step now declares shell: bash, and the checksum records the bare file name it is verified against. The contract is enforced by tests/ci-workflows/release-pipeline-contract.test.ts: every multi-line script on a Windows runner must declare an explicit shell, and a checksum argument must not carry a directory prefix the verifier cannot resolve. Both were driven red against the unfixed workflow before this change.
publish declared needs: validate-dispatch only, so npm publication and the GitHub release could proceed while desktop packaging was still failing, which is how a version becomes public with no app attached. The job now waits for package-standalone and package-desktop, matching the ordering attach-release already demanded of it. The release-pipeline contract test asserts the publish and attach-release dependency sets, and the long-standing assertion in ci-workflows.test.ts that encoded the old single-dependency shape is updated to the gated one. Driven red before the change.
… the desktop shell The lifecycle workflow and the release gate both keyed on src/service.ts, but the service implementation is the eleven-file src/service/ directory, and neither covered desktop/, whose shell packages and launches the service. A change to launchd.ts or windows-scheduler.ts alone produced no lifecycle run, and a release touching those files would have gated on evidence that never ran. Both triggers and the release gate regex now cover src/service/** and desktop/**, kept in sync in both files. The contract test derives the service module and desktop surface lists from the tree itself rather than restating them, so a new file that falls outside the gate fails the test. Driven red before the change.
…tity isOcxCommandLine matched ocx and ocx.cmd but not ocx.exe, even though scripts/build-standalone.ts and desktop/scripts/prepare-sidecar.ts both emit ocx.exe on Windows targets and the bundled sidecar is copied as ocx-<triple>.exe. The predicate feeds pid identity, so the shipped Windows binary was the one shape the identity check did not know: a service restart or a receipt-backed stop could not tell its own process from a foreign one. The executable token now accepts .exe as well as .cmd, so a quoted install path with spaces and any letter case matches, while lookalikes such as not-ocx.exe, myocx.exe and ocx.exes stay foreign. Driven red in tests/service/process-state.test.ts before the change.
ProxyClient sends the admin token to 127.0.0.1, but reqwest resolves system proxy configuration by default, so a machine-wide proxy would receive a credential that is only ever meant for loopback. The builder now disables proxy resolution explicitly. tests/clients/desktop-proxy-direct-transport.test.ts asserts the builder carries no_proxy(), driven red before the change.
6903852 to
74cd7ec
Compare
리뷰 · 우선순위 73 / 80이 PR은 릴리스가 깨지던 여섯 곳을 하나씩 고치고, 다시 깨지면 CI가 빨간불을 켜게 테스트를 붙입니다. Windows 패키징에서 bash 문법을 PowerShell이 읽던 칸에
메인테이너의 판단이 필요한 지점 릴리스 자동화 변경이라 MAINTAINERS.md상 보안 리뷰 스탬프가 필요합니다. 시크릿·permissions는 안 건드렸고, 프록시 우회는 자격 증명이 밖으로 안 나가게 막는 쪽입니다. 너의 추천 여섯 결함 수리와 계약 테스트 방향은 맞습니다. 사이드카 파일명이 프로세스 소유권 경로에 실제로 들어오면 이 댓글은 grok-bot이 작성했습니다 |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes after explicit workflow/release security review of exact head 74cd7ec9645481f4bc60985fcc8907d1a48712d2.
The workflow fixes are directionally correct: permissions and secret scope are unchanged, package publication is ordered after both packaging jobs, the checksum chain is coherent, and .no_proxy() narrows the desktop admin-token transport to loopback. One functional blocker and one documentation obligation remain:
isOcxCommandLinenow recognizes only bareocx.exe/opencodex.exe. The shipped Windows desktop sidecar is namedocx-<target-triple>.exebydesktop/scripts/prepare-sidecar.ts, so the process-identity path this PR claims to fix still classifies the packaged sidecar as foreign. Accept the exact trusted sidecar shape while continuing to reject lookalikes (not-ocx-*,myocx-*, extra suffixes), and add positive/negative tests using the actual generated filename.- This changes the owned
src/config/process-identity contract but does not update the mappedstructure/runtime.mdandstructure/config.mdSSoT. Document the accepted Windows executable shapes and boundary in both.
Please also decide whether desktop/** is intentionally the service-lifecycle trigger or whether it should be narrowed to runtime-relevant desktop paths; broad coverage is safe but materially increases CI cost. Replacement exact-head CI must be fully green before integration.
2 participants
Summary
Lane E of the app-runtime-ownership unit (decision D9, part one) fixes six confirmed release-pipeline defects and adds workflow-contract regression tests so each one fails CI if it regresses. Ordered commits, one fix each:
Rename release assetsstep uses backslash continuations and"$VAR"expansion with noshell: bash, and the workflow sets nodefaults.run.shell, so a Windows runner reads the script as PowerShell. The step now declaresshell: bash.sha256sum "dist/ocx-…"into the checksum file, butattach-releaseverifies withshasum -a 256 -c ./*.sha256from insidedist/release, where nodist/directory exists. The step now checksums the bare file name from the artifact directory, and the contract locks the whole path chain: bare recorded name, bare redirect target, thecdinto the artifact directory, thedist/ocx-*.sha256upload glob, and the flattened download beside the verifier.publishdeclaredneeds: validate-dispatchonly, so npm publication and the GitHub release could proceed while desktop packaging was failing, which is how a version becomes public with no app attached.publishnow waits forpackage-standaloneandpackage-desktop. The pre-existing assertion intests/ci-workflows/ci-workflows.test.tsthat encoded the old single-dependency shape is updated to the gated one.service-lifecycle.ymland the release gate both keyed onsrc/service.tswhile the implementation issrc/service/**, and neither covereddesktop/, whose shell packages and launches the service. Alaunchd.ts-only change produced no lifecycle run, and a release touching those files would gate on evidence that never ran. Both triggers and the gate regex now coversrc/service/**anddesktop/**, kept in sync. The contract test derives the service-module and desktop-surface lists from the tree itself rather than restating them.ocx.exewas not recognised as an opencodex process.isOcxCommandLinematchedocxandocx.cmdbut not theocx.exethatscripts/build-standalone.tsanddesktop/scripts/prepare-sidecar.tsboth emit on Windows targets, and this predicate feeds pid identity — the shipped Windows binary was the one shape the identity check did not know. The executable token now accepts.exe: a quoted install path with spaces and any letter case matches, while lookalikes such asnot-ocx.exe,myocx.exeandocx.exesstay foreign.ProxyClientbuilds its reqwest client with the system-proxy default enabled while attaching the admin token to loopback requests. The builder now calls.no_proxy(), and the contract test strips comments before asserting it, so a commented-out.no_proxy()cannot satisfy the check.Security review: this PR changes release automation (
.github/workflows/release.yml,.github/workflows/service-lifecycle.yml), which MAINTAINERS.md requires to carry explicit security review. The change is structured so a reviewer can assess it in separable parts: nopermissionsblocks change, no secrets are added, renamed, or re-scoped, and the publication-ordering commit (2) is independent of the packaging-correctness commits (1, 3). Commit 6 removes a credential-exposure default rather than adding a capability.No GUI change; no screenshot required.
Verification
Local execution checks: NOT RUN (lane policy — no local
bun test,bun run test,bun run test:changed,bun run typecheck, builds, installs, orocxexecution; hosted CI at the exact head SHA is the PR-ready gate and is reported separately).Static verification performed instead:
Rename release assetsstep contains noshell:key, soexpect(step.shell).toBe("bash")fails withundefined; the old checksum arguments aresha256sum "dist/ocx-…", failingnot.toContain("/"), and the oldcd ../../..fails thecd ../..transition assertion; the old scalarneeds: validate-dispatchfails the three-job sorted equality; the old gate regex has nosrc/service/.*ordesktop/.*alternative, sosrc/service/state.tstests false; the old predicate(?:ocx|opencodex)(?:\.cmd)?cannot matchocx.exe— afterocx,.exematches neither\.cmdnor the trailing boundary; the oldproxy.rsbuilder contains no.no_proxy().dist/ocx-*.sha256, theupload-artifactglobdist/ocx-*.sha256matches it, thedownload-artifactstep usespattern: standalone-*withmerge-multiple: trueintodist/release, and the verifier runsshasum -a 256 -c ./*.sha256fromdist/release— the bare recorded name resolves at every hop.service-lifecycle.ymltrigger literal (includingsrc/service/**anddesktop/**) matches the release gate regex whilesrc/router.tsand docs-site paths stay excluded; every new.exepositive/negative case traces correctly through the final predicate; reqwest is pinned at 0.12.24 andClientBuilder::no_proxy()exists there, placed before.build();git diff --checkclean; no host names, IP addresses, SSH accounts, or absolute user paths in added lines. Its findings (a literal loopback IP in a test comment, checksum-chain joints left unasserted, a comment-tolerant source match) are folded into the final commits.scripts/test-layout/layout.json(explicit) andtests/fixtures/test-layout-expected.json; the only touched file covered bytests/fixtures/file-size-baseline.jsonistests/ci-workflows/ci-workflows.test.tsat 5614 lines against a 5628 cap;git merge-tree --write-tree origin/dev HEADproduces a clean merge result.Checklist
Summary by CodeRabbit
Bug Fixes
.exevariants and quoted installation paths.Release Improvements