Conversation
…enerations Give agents an explicit notify path independent of Stop hooks, with durable callback identity across updates, portable setup, and an isolated local E2E matrix. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Important Review skippedToo many files! This PR contains 314 files, which is 164 over the limit of 150. To get a review, reduce the PR to 150 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (314)
You can disable this status message by setting the 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 |
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. |
Keep isolated flow identity/rollback by default. Notification Center and production bundle ID require AGENT_NOTIFY_DARWIN_E2E=1 so CI and HOME-isolated runs do not touch operator LS/NC. Co-authored-by: Cursor <cursoragent@cursor.com>
![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
https://github.com/777genius/agent-notifications/blob/68d894a80a05d84a251734f4e2d9b32ac4e14c2b/internal/installruntime/path_unix.go#L267-L270
When the operator-editable agent-notifications.json or another managed control document is unexpectedly large, readRegularFile allocates the entire file before any of the callers apply their 64 KiB JSON budget. ReadPolicySnapshot reaches this reader on every notify/status request, so a malformed or accidentally huge policy can exhaust process memory instead of being rejected as invalid; use a size check and limited reader here.
ℹ️ 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".
Compensate a failed Publish by removing only a consumer this commit just added, and fence that undo to the generation just written. A retry must not drop a working portable registration. Co-authored-by: Cursor <cursoragent@cursor.com>
…app alias Ubuntu retirement tests now follow published generations instead of live-path exchange, native promotion tests skip unsupported platforms, and isolated E2E no longer fatals without the gitignored helper. Hook discovery retargets ClaudeNotifier.app at the active generation without swapping callback inodes. Co-authored-by: Cursor <cursoragent@cursor.com>
…ead helper The flow now covers both configure orders, partial second-client failure, ClaudeNotifier.app alias retarget, A→B rollback, and LaunchServices cold-start of the test bundle. Notification Center send stays opt-in so default runs do not post to the operator. Co-authored-by: Cursor <cursoragent@cursor.com>
§8.1.2 requires the existing installer entrypoints, not only the Go orchestration tests. Bootstrap and Claude init now pass --configure-notifications after a successful install, with route flags rejected until that opt-in is present. Co-authored-by: Cursor <cursoragent@cursor.com>
setup-codex and bootstrap were calling configure with the source plugin root. After install that directory is disposable, so the owned ledger consumer is the copied runtime under CODEX_HOME. Darwin now requires qualified native before that hook commit when configure is requested. Co-authored-by: Cursor <cursoragent@cursor.com>
CI never reached the isolated flow: go fmt check failed on the LaunchServices bundle-id slice. Co-authored-by: Cursor <cursoragent@cursor.com>
…t helper macOS CI has no gitignored .build helper, so the isolated flow skipped A→B entirely. An inert clang fixture with the test bundle ID keeps generation identity, alias, rollback and cold-start on the CI path. Co-authored-by: Cursor <cursoragent@cursor.com>
Installers now register agent-initiated notify unless --skip-agent-notify is passed. Configure failures leave hooks in place, and the unreleased --configure-notifications alias is removed. Co-authored-by: Cursor <cursoragent@cursor.com>
Parse and reject unknown routes and the retired alias before curling the installer, and keep setup-codex next-step text after a successful configure. Co-authored-by: Cursor <cursoragent@cursor.com>
setup-codex now configures by default, so bootstrap must pass --skip-agent-notify during install_codex and configure once afterward. Co-authored-by: Cursor <cursoragent@cursor.com>
777genius
left a comment
777genius
left a comment
There was a problem hiding this comment.
Follow-up review: the supplied fixes are not ready to be called closed end-to-end. I found 6 P1 and 4 P2 issues below.
Scope: this review covers the uncommitted fix snapshot based on bd72d5244928f19fd7b55b2b18b77d280c546334, patch SHA256 a293f38d3fa1ba54def680dea8aacad124fdf5b1e301c4ede67312bbf95746f0. The remote PR still points to that base commit. Line numbers below refer to the supplied local fixes, so I am recording them in the review body rather than attaching them to different bytes in the current diff. No implementation changes were committed or pushed during this review.
P1
-
Blob cleanup can delete unrelated files through a directory symlink.
internal/installruntime/recovery.go:127-135, called unconditionally fromtransaction.go:653:os.ReadDir(control/transaction.blobs)follows a symlink, thenos.Remove(dir/name)deletes entries in its target. A normal Commit with no blob payload reproduced this: an unrelated sentinel in a separate disposable directory disappeared and Commit returned nil. Use a verified, pinned directory and handle-relative cleanup of owned entries; refuse symlink/reparse directories. -
Fresh installation rejects the shipped skill, and acquisition registers temporary paths in the live ledger.
bin/install.sh:2012-2014,cmd/claude-notifications/install_runtime.go:263,bin/bootstrap.sh:1313,1356: the extracted source bundle already hasskills/agent-notify/SKILL.md, but the adapter requires that file to match an existing ledger. Fresh install fails withcanonical skill is not an unchanged owned regular file. I reproduced this with the actual shell staging function and a full CLI build using the repository's pinned dependencies. A minimal fixture without that pre-existing skill then exposes the next failure: acquisition registersclaude-hooksunder the temporary bundle, realsetup-codexadds the final consumer, and deleting the temporary bundle invalidatesReadInstalledSnapshot. Full CLI configure then fails at preflight. An existing Claude consumer at another path instead hits the relocation check. Acquisition must avoid live registration; publish and adopt shipped files at the final consumer destination. -
Windows shell changes a managed BAT immediately after its commit.
cmd/claude-notifications/install_runtime.go:162generatesclaude-notifications.batwithoutset AGENT_NOTIFICATIONS_LAUNCHER=claude-notifications;bin/install.sh:2150subsequently calls the shell generator, which adds that line at:1298and replaces the file outside the transaction. The real snapshot reader then reportsinstalled file fingerprint mismatchfor the BAT. The same shell writer is called on the existing-binary path. Use one transaction-owned launcher representation. This was reproduced with the production adapter/kernel and actual shell generator on Linux using a Windows entry name, without executing a Windows binary; Windows filesystem behavior remains a separate gate. -
Offline fallback deletes the working older executable.
bin/install.sh:1212-1216, reached from:2120: an existing binary without the new marker failscheck_existing; an ordinary non-forced offline run then callsverify_executableon the live path. Even after a successful--version, the new marker check removes that binary. The isolated production verifier probe returnedverifier_exit=1andold_binary=DELETED. Reject historical writers while retaining working bytes until a replacement is available. -
The repair exemption also permits overwriting existing foreign edits.
internal/installruntime/transaction.go:328-330: every path inr.Filesbypasses the persisted ledger identity check.StageFilesrecords an already edited file asFile.Before, so the later CAS only confirms those freshly captured foreign bytes. Reproduction: installoriginal, edit toforeign, stageupgrade, Commit succeeds and overwritesforeign. Restrict the deleted-launcher repair exemption to the intended missing identity; preserve ownership checks for existing changed files unless explicit takeover was selected. -
Navigation none now silently grants unknown-caller consent.
cmd/claude-notifications/agent_notify_setup.go:213-217setsAllowUnknownCaller=truewhen both consent flags are omitted. Bootstrap supplies this route automatically, and reconfiguration can replace a previously denied consent. Keeping the admission expression unchanged does not preserve the effective policy when its input is changed this way.docs/commands/setup-notifications.md:144-151explicitly requires unknown-caller consent even for none. Known remote/headless callers are still rejected; the regression concerns indistinguishable callers. Obtain the separate consent in setup, or explicitly agree and document a change to this trust contract instead of treating it as a mechanical default-on fix.
P2
-
Missing blob is interpreted as missing transaction marker.
internal/installruntime/transaction.go:196-203:readTransactionFilenow loads both marker and payloads, but anyos.IsNotExisterror is treated as no pending transaction. After a post-marker fault on a 2 MiB payload, removing the blob and retrying with no new files returned success at generation 1 and removed the original marker without installing its asset. Only ENOENT opening the marker itself should mean no pending decision. Missing dependencies must stop mutation and preserve the marker. -
Custom Codex home still breaks fresh default-on bootstrap configuration.
bin/bootstrap.sh:1033-1034: default route injection checks whether allCONFIGURE_ARGSare empty. With only--codex-home /abs/path, the array contains the home option but no route. The configure parser leavesRoute=nil, and fresh enable is rejected by setup withroute_required. A direct probe ofselect_productconfirms the missing route. Determine whether route options were supplied independently of home/formatting options. The new binary-path propagation is correct, but this full custom-home scenario remains incomplete. -
Rollback accepts and forgets an ambiguous already-published native B.
internal/installruntime/recovery.go:250-260: the fallback verifies intact A without requiring B to be absent. Publish B, fault at the native boundary, change B's tree, then roll back: the real kernel succeeds, removes the pending marker, and returns a Published list containing only A. The proposed fix handles absent B but also removes the refusal for present, unrecognized B. Distinguish those states and retain the published identity or refuse ambiguous recovery. Reproduced with inert native bundles that were never launched. -
A blob over 32 MiB is still accepted into an unrecoverable transaction.
internal/installruntime/recovery.go:76,114: the writer persists the complete payload, while the reader still uses the 32 MiB managed-file cap.StageFilesdoes not cap new payload bytes. A 33 MiB payload reached the durable post-marker fault; retry failed withcorrupt installation transaction: managed input exceeds size limit. Enforce a consistent bound before publishing the decision or support the accepted size in bounded recovery storage. The 25 MiB case now passes.
Validation
- PASS with the full repository dependency graph: all six
TestSetupCodexE2E*tests,TestAgentNotifyIsolatedInstallFlowE2E,TestNotificationBootstrapOffline,TestNotificationInitOfflineBranch, andTestNotificationConfigureParserAndSetupOptIn. - PASS: reboot burst without Collect, unpublished-B rollback, 25 MiB marker round-trip and real crash recovery, and existing transaction/promotion/ledger recovery boundaries.
- FAIL: focused reproductions described above. Full CLI + actual shell staging independently reproduced the fresh shipped-skill failure and the temporary-ledger/configure failure.
- Installer E2E is not green. The root run hits the expected permissions-fixture limitation. An unprivileged rerun still exits early; the separate unprivileged
--mock-onlyrun failstest_mock_download_success(exit 1, no binary/launcher). Its mock payload has neither the new writer marker nor a managed publication implementation. - Isolated Linux server execution only: disposable HOME/TMPDIR/XDG paths, GOMAXPROCS=2, Go
-p 1, no real profiles, notifications, agent sessions, or release publication. The full CLI/E2E uses the repo's pinned dependencies. Kernel fault probes use an external minimal modfile with pinnedx/sys v0.41.0; the installer's initial independent probe used cached v0.35.0, and the important adapter findings were rechecked with v0.41.0/full CLI. - Native Windows filesystem behavior, macOS signing/GUI/callback delivery, and a fresh CI run for the uncommitted fixes are not established by these Linux tests. The old PR-head CI is still red and does not validate this local snapshot.
The passing scenario tests are useful, but the production installer-to-ledger boundaries above still prevent an end-to-end approval.
Refuse symlink blob cleanup, foreign edits, and silent unknown-caller defaults, keep Codex acquisition off the live ledger, and require explicit none-route consent on default-on install. Co-authored-by: Cursor <cursoragent@cursor.com>
777genius
left a comment
777genius
left a comment
There was a problem hiding this comment.
Follow-up review of 86a8b9b6998c9b48d94172662ffa7c70c71b2e06: 1 P1 and 6 P2 comments attached to the current diff. The main blocker is updating an installed Codex runtime outside the installation transaction.
The earlier missing-blob, oversized-payload, foreign-replacement, changed-content native rollback and symlink-cleanup reproductions now refuse correctly. Shipped-skill adoption, fresh Codex acquisition followed by durable setup/source cleanup, 25 MiB recovery, and reboot rate-limit refusal recovery passed the focused checks. The new findings cover remaining cases rather than repeating the fixed reproductions.
Validation used disposable Linux server environments, the full CLI built from this commit, the repository's pinned dependencies, actual shell/Go installation boundaries, parser/configure composition, and kernel fault probes. Acquisition transport used local checksum-verified assets; optional host integrations and the independent bootstrap config-preflight seam were inert. No real profiles, agent sessions, notifications, native apps, or releases were exercised. The installer review's smaller production-adapter probe was independently checked with the full CLI for the P1 update defect. Native directory probes used inert Linux fixtures, not macOS GUI qualification.
Current CI remains red: Ubuntu fails the new real-installer fixture; Windows includes drive-path errors, Unix-specific fixtures, and test executables exceeding the 32 MiB staging cap; lint reports 60 issues. Both macOS Go jobs, Swift notifier tests and landing verification passed. These results do not yet establish end-to-end readiness. No production code was changed during this review.
Disposable copy is only for validated acquisition; live Codex refresh must commit fingerprints so configure still matches the runtime. Co-authored-by: Cursor <cursoragent@cursor.com>
777genius
left a comment
777genius
left a comment
There was a problem hiding this comment.
Round 4 review of ff6b463. Six P2 findings remain: four runtime/recovery issues and two deterministic test-fixture failures. No new P1 found in this review.
The prior P1 Codex live-update bypass is fixed: a real CLI bootstrap followed by verified live updates advanced the ledger 1 -> 2 -> 3, retained matching fingerprints, and kept both launchers pointed at the updated binary. The previous unknown-blob deletion and original native directory-substitution reproductions now pass. The adjacent interrupted-rollback case is described inline.
Hosted validation used isolated test homes/projects and the exact commit. The CLI update probe used local transport and optional OS-integration stubs; setup-codex and the managed writer were the real CLI. All focused TestSetupCodexE2E* cases passed, including TestSetupCodexE2EInstalledLaunchersSurviveReplacement (3.27s); the earlier build timeout was not reproduced. The focused CLI run still fails TestNotificationBootstrapRealInstaller for the concrete config-fixture issue below. The full Linux installruntime suite and the previous kernel regression probes passed; new negative probes reproduced the rollback finding.
Current-head CI: macOS Go 1.25/1.26 and Swift tests pass. Ubuntu and Windows are red. Lint additionally reports 60 diagnostics, mostly unchecked errors and staticcheck issues; these need cleanup before a green gate. Windows Go 1.25 also has process-boundary failures beyond the deterministic issues below, so this is not a complete diagnosis of every CI failure.
These checks establish installation/update and recovery behavior, not a live desktop notification after a real client/session restart. Linux MCP configure still correctly reports its current unsupported_platform limitation.
Skill Prepare and missing launchers were blocked by unrelated fingerprints or out-of-band copies, interrupted native rollback dropped published B, and Windows directory sync left prepared config unready. Co-authored-by: Cursor <cursoragent@cursor.com>
777genius
left a comment
777genius
left a comment
There was a problem hiding this comment.
Round 5 review of 2e209ce: six P2 findings attached, consisting of three behavior/recovery defects and three deterministic test-fixture defects. No new P1 found in this bounded review.
The previous missing-platform-binary repair now succeeds, including ordinary and forced installer execution. Normal missing-launcher repair commits through the kernel when admissible. The earlier native-only interrupted rollback and directory-substitution probes now pass. The remaining findings exercise refused repair and interrupted rollback with changing ordinary/config payloads.
Validation:
- Full CLI built from this commit with the repository's pinned dependencies. Actual installer/setup/update probes in disposable Linux environments retain valid ledgers through successive updates, missing-binary repair, and admissible launcher repair. An independent production-adapter integration also covers actual config preflight, a custom Codex home, and acquisition source cleanup.
- All focused TestSetupCodexE2E cases pass, including InstalledLaunchersSurviveReplacement (2.81s). The earlier CLI build timeout was not reproduced. The focused CLI run still fails TestNotificationBootstrapRealInstaller for the version mismatch described inline. Changing only the external test fixture's 1.42.0 literals to 1.43.0 makes that test pass with real preflight enabled.
- The full Linux installruntime suite and previous recovery/ownership probes pass. New fault probes reproduce the adjacent repeated-rollback defect. Independent full-CLI and production-adapter probes reproduce the refused-launcher repair defect.
- Current-commit CI remains red: Ubuntu fails the real-installer fixture; Windows includes published-config flush and unsupported client-projection failures; both macOS jobs fail the shell transaction fixture before their Go suites. Lint also remains red. Swift notifier tests and landing verification pass. This is not an exhaustive diagnosis of every failed job.
Tests used isolated hosted test homes/projects, local asset transport, and inert native fixtures/optional host integration seams. No real profiles, agent sessions, desktop notifications, or releases were exercised. In particular, installation/recovery checks do not establish live MCP/hook notification delivery after a real session restart or OS reboot. No production code was modified during this review.
… ledger A managed refresh refusal must not publish an untracked symlink, and retrying rollback must complete the persisted reverse instead of flipping ordinary files back to the interrupted upgrade. Co-authored-by: Cursor <cursoragent@cursor.com>
777genius
left a comment
777genius
left a comment
There was a problem hiding this comment.
Round 6 review of d5f13af: six P2 findings attached. They concern test correctness and fixture compatibility. No additional production defect or new P1 was established in the bounded re-review.
The round-5 production findings are fixed:
- Refused managed launcher refresh now fails the installer and leaves the missing launcher absent. A full-CLI reproduction then restores only the foreign-edited skill and successfully updates the runtime, without the earlier shell-created drift.
- All eight previously failing repeated-rollback cases pass. The independent kernel review also passes 30 new cases covering small/2 MiB/9 MiB payloads, regular replacement/removal/link transitions/new-file rollback, and repeated promotion/ledger interruptions. The persisted reverse decision remains unchanged across retries, restores A's ordinary/config/consumer state, retains the selected native generation, cleans recovery artifacts, and allows subsequent repair. Full Linux installruntime and prior refusal/corruption probes pass.
- Windows prepared-config publication no longer fails FlushFileBuffers; the new CI reaches the later mode assertions described inline.
Full-CLI focused validation is green: TestNotificationBootstrapRealInstaller passes with real preflight (11.02s), and all selected TestSetupCodexE2E cases pass, including InstalledLaunchersSurviveReplacement (2.62s). The focused CLI run completes in 18.97s. Actual setup/update and independent adapter integrations validate custom Codex home, source cleanup, successive updates, missing-binary repair and admissible launcher repair. The shell transaction, adapter and config-preflight suites pass.
Current-head CI is still red:
- Both Ubuntu Go suites pass, then scripts/config_e2e_test.py fails 7 of 16 scenarios because its copy-only installer no longer satisfies the managed writer contract.
- Both macOS Go suites pass, as does the repaired shell transaction fixture. Their later installer E2E step stops at the fake existing Windows binary.
- Both Windows versions fail the canonical-skill removal expectation and POSIX mode assertions. Windows 1.26 additionally exposes the deadline-clock and manual-rename test races described inline.
- Lint still reports 60 diagnostics (50 errcheck, 10 staticcheck). Swift notifier tests and landing verification pass.
Correction to the previous review's CI wording: macOS runs its Go tests before the shell fixture steps. A later shell failure does not mean the Go suite was skipped.
Validation used hosted disposable test homes/projects, pinned dependencies, local transport and inert optional OS integrations/native fixtures. Root used the full CLI; the independent installer review used unchanged production adapter/kernel files with an external dispatcher. The deadline reproduction changes only external test scheduling: 200 ordinary repetitions pass, while a controlled initialized-frame interleaving reproduces the CI failure; synchronizing initialization makes that control pass. No production code, real profiles, agent sessions, desktop notifications or releases were changed/exercised. Live notification delivery after session restart or reboot remains outside these checks.
…riter publication Tests were asserting POSIX modes, copy-only install.sh, and an uncoordinated MCP clock that the kernel no longer matches; they now expect platform-true permissions, launcher refresh, protocol-compliant publication, and a handshake-armed deadline. Co-authored-by: Илия <iliyazelenkog@gmail.com>
777genius
left a comment
777genius
left a comment
There was a problem hiding this comment.
Round 7 review of 63c93d0: two remaining P2 fixture findings below. The six round-6 findings are fixed. This head changes only six test files; production code and dependencies are unchanged from the previous reviewed head, and no new production defect was established.
Verified progress:
- Current-head Go test steps pass on Ubuntu, macOS and Windows. The Windows canonical-skill cleanup, permission expectations and snapshot-writer retry now pass.
- scripts/config_e2e_test.py now uses the production installer and passes all 16 scenarios in current-head Ubuntu CI, including Codex/both installation, config repair and concurrent config changes.
- The repaired existing-Windows fake passes its assertions, along with adjacent historical/wrapper/sh hook rejection cases.
- A focused hosted run of the changed CLI/config/kernel tests passes. The new synctest deadline test passes 200 repetitions on the current head.
- Prior rollback and launcher-refusal production checks remain applicable to unchanged source; the expensive kernel matrix was not rerun.
The remaining installer E2E failures have two different fixture causes: the shared mock writer cannot refresh its own published directory, and the real-Windows executable fixture bypasses initial managed registration. Linux/mock controls reproduce the former and pass after changing only the external fake writer. The latter is visible in native Windows CI and reproduced with the unchanged full production CLI, an inert Windows entry name and simulated shell platform detection on Linux: no consumer gives refresh refusal; proper initial kernel registration then makes installer execution and exec-form hook configuration pass. This does not claim native Windows execution of the Linux control.
CI remains red in installer E2E and lint. Lint still reports 60 diagnostics (50 errcheck, 10 staticcheck). Swift notifier and landing checks pass. Fix the fixtures while retaining the managed writer and ownership refusal checks.
All added probes use disposable hosted test homes/projects and inert/local fixtures. No production files, dependencies, real profiles, agent sessions, desktop notifications, commits or releases were changed/exercised. Root's CLI control reuses the previous full build because all production inputs are unchanged. The independent review's attempted MCP test could not resolve the pinned SDK in its offline cache; the root's current-head 200-repeat run and CI supply that test evidence. Live notification delivery after a real session restart or reboot remains outside this review.
The shared mock copied a live tree onto itself and the Windows exec fixture had no consumer, so the real installer correctly refused repair. Co-authored-by: Cursor <cursoragent@cursor.com>
777genius
left a comment
777genius
left a comment
There was a problem hiding this comment.
Round 8 review of d3233d3: two P2 test/fixture findings attached. No additional production defect was established in this bounded re-review. The delta from round 7 changes only bin/install_e2e_test.sh; production code and dependencies are unchanged.
The shared mock writer fix is effective: current-head Ubuntu 1.25/1.26 and macOS 1.26 checks pass. Initial managed registration has also been added to the real-Windows fixture, but both native Windows jobs now stop there with managed input exceeds size limit, before the exec-form hook assertions.
The macOS 1.25 failure is TestSDKSaturationCancellationFloodAndDrain at lifecycle_test.go:113, not a CLI build timeout. An isolated hosted Go 1.25.13 run passes 100 ordinary repetitions. A review-only I/O wrapper then reproduces the missing initialization barrier in 10/10 controlled runs: the client has read the initialize response, seven handlers start, and call 8 receives busy. Wrapping the original saturation test in synctest and waiting for initialization to settle passes 100 repetitions while preserving the saturation, cancellation-flood and drain assertions.
The Windows finding uses the native CI logs. A supplemental Linux cross-build could not complete because the full CLI requires CGO/malgo and the host has no Windows C toolchain; no Windows artifact size comparison or native Windows execution is claimed from that probe.
Current head is still not merge-ready: Windows 1.25/1.26, macOS 1.25 and Lint are red. Lint still reports the previously documented 60 diagnostics (50 errcheck, 10 staticcheck); those are not duplicated as new inline findings.
All new probes ran in a disposable hosted sandbox with fake notification backends. No repository production files, real profiles, agent sessions or releases were changed. Live notification delivery after client restart or OS reboot remains unverified by this review.
The E2E fixture was committing the running Windows image in place, which the kernel refuses; split stage from target and unblock lint/MCP CI. Co-authored-by: Cursor <cursoragent@cursor.com>
Unstripped go build exceeded the 32 MiB managed-file cap, so first registration never reached the installer or exec-form hook checks. Co-authored-by: Cursor <cursoragent@cursor.com>
…ging Fresh Codex copies omit .app trees from StageFiles, so hook discovery needs stable aliases for both ClaudeNotifier.app and terminal-notifier.app instead of falling back to beeep. Co-authored-by: Cursor <cursoragent@cursor.com>
777genius
left a comment
777genius
left a comment
There was a problem hiding this comment.
Round 9 review of 6c1e743, including all three commits since d3233d3. Two findings attached: P1 public installer/release compatibility and P2 optional legacy alias regression.
The previous two P2 findings are fixed. Current-head Go 1.25/1.26 checks on Ubuntu, macOS and Windows, Lint, Swift tests and landing verification are all green. A fresh hosted run of installruntime, codexsetup and agentnotify/mcp also passes on this head.
Release-channel validation found a separate gap that CI's built-from-head fixtures do not cover: the current installer rejects the actual latest published v1.43.0 Linux binary. Its SHA-256 is 680322cef575327376443a4d900ebbcb81c44d3aa673d30ec85b0fd97ed1db94, matching the published checksums.txt. With those unchanged assets supplied to INSTALL_STAGED_ASSETS in a disposable home/target, the installer passes checksum and --version checks, then exits 1 at the managed-writer floor. The public bootstrap currently combines main's install.sh with latest-release assets, so this matters before exposing this commit on main, not only for a future SDK rollout.
An independent source review identified the new optional-legacy-alias refusal. A focused sandbox probe confirms that preparing the available modern alias succeeds, while adding a pre-existing unmanaged terminal-notifier.app symlink makes NativeAlias reject the whole operation. The foreign link remains unchanged; the regression is unnecessary refusal, not clobbering.
The future UAP SDK/wizard plan is not a prerequisite for merging or releasing this PR. Close the current findings and prove the chosen public channel/artifact pairing first. Before claiming a complete notification release, also finish the selected real-client/OS qualification and make partial configure outcomes explicit; the current bootstrap/setup-codex behavior intentionally still reports hook installation success after agent-notify configuration failure.
Validation used disposable hosted source, homes, staged public assets and inert native fixtures. No real user profiles, client sessions, GUI notifications, reboot, commits, merge or release were performed. This does not establish live delivery after client restart/reboot or universal MCP support on Windows/Linux.
main's installer rejects the current v1.43.0 binary at the managed-writer floor. Fresh public installs now fetch install.sh from the same tag as the binary, while already-managed ledgers keep the current writer and leave an unrelated legacy notifier symlink in place. Co-authored-by: Cursor <cursoragent@cursor.com>
777genius
left a comment
777genius
left a comment
There was a problem hiding this comment.
Reviewed 629f538 against 6c1e743.
The optional legacy alias fix is covered and passed the focused TestNativeAlias suite. The bootstrap product fixtures also pass. All six OS/Go matrix jobs, Lint, Swift tests and landing verification are green on this head.
One P1 remains in the public release pairing. I reproduced it in a new Linux sandbox using the unchanged bootstrap from this head, the actual v1.43.0 tagged install.sh/source archive, and the published Linux binary verified against release checksums. The tagged installer succeeds, but bootstrap then fails before Codex registration on an unsupported CLI flag. The HTTP fixture only mirrors acquisition and provides a fake prerequisite codex command; no real user profile or agent session was used.
This review does not establish native GUI delivery or restart/reboot E2E. The future UAP SDK/wizard is a separate delivery scope and is not a prerequisite for fixing this compatibility issue.
| printf '%s\n' "$MANAGED_INSTALL_SCRIPT_URL" | ||
| return 0 | ||
| fi | ||
| printf '%s\n' "$BOOTSTRAP_RAW_CONTENT_URL/${BOOTSTRAP_TAG}/bin/install.sh" |
There was a problem hiding this comment.
[P1] Pair the setup-codex invocation with the released CLI too
On a fresh install, this branch now correctly selects v1.43.0/bin/install.sh, but install_codex still unconditionally calls the resulting v1.43.0 binary with setup-codex --skip-agent-notify (lines 1427-1429). That released parser does not support the flag. Running the unchanged bootstrap with the real tagged installer/source and checksum-verified released Linux binary reaches setup-codex: unknown option: --skip-agent-notify, exits before registration, and leaves Codex hooks absent. This affects --product codex and both, even when bootstrap itself is invoked with --skip-agent-notify. Keep the legacy release invocation compatible, or route to a compatible published binary before passing the new flag; also gate later configure calls on actual release capabilities. Add a pairing regression using the released CLI contract, since the current fake binary accepts arguments that v1.43.0 rejects.
There was a problem hiding this comment.
Fixed in 8fbd174. install_codex probes the published binary's --help/help and only forwards --skip-agent-notify when that flag is advertised. v1.43.0 rejects the unknown option, so the fresh pairing path now omits it. macOS Bash 3.2 + set -u cannot expand an empty "${skip[@]}" , so the capable path uses set -- --skip-agent-notify "$@". Product fixtures model the released parser rather than this branch's CLI.
There was a problem hiding this comment.
Re-checked on current HEAD (89d8524) against the published Darwin arm64 v1.43.0 binary (SHA-256 9d193a1e10a6020f7bd99fab32601492dc9d85ec087b4fe75684842d5ce4a3d2, matches checksums.txt). --help does not advertise --skip-agent-notify; setup-codex --skip-agent-notify still exits 1 with setup-codex: unknown option: --skip-agent-notify. HEAD cli_has_setup_codex_skip_agent_notify is false on that binary, and run_codex_setup --dry-run forwards only --dry-run. Isolated HOME, checksum-verified release asset, no user profile or GUI session.
v1.43.0 rejects --skip-agent-notify, so pairing the tagged installer still failed before Codex hooks were registered. Probe advertised help before passing new flags or calling setup-notifications, and the product fixture now models that released parser. Co-authored-by: Cursor <cursoragent@cursor.com>
Loaded macOS runners can deliver the signal before NotifyContext is armed, so the helper dies with the default disposition instead of the graceful unknown exit. Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
Summary
clientInfo.--agent-notify,--navigation noneunless a route is supplied). Pass--skip-agent-notifyfor hooks only. The landing installer checkbox is on by default.Test plan
GOTOOLCHAIN=local GOMAXPROCS=2 HOME=/tmp/agent-notify-e2e-home TMPDIR=/tmp/agent-notify-e2e-tmp /opt/homebrew/bin/go test -p 1 -count=1 -timeout=180s ./cmd/claude-notifications/ -run TestAgentNotifyIsolatedInstallFlowE2E./internal/installruntime/ -run 'TestNativeExactHeadGenerationsPreserveCallbackIdentity|TestNativeGenerationRemainsExecutableAfterUpdate|TestRetireNativePreservesPublishedPredecessor'./internal/hooks/ -run TestHandler_NotificationsDisabled./cmd/claude-notifications/ -run 'TestNotificationConfigureParserAndSetupOptIn|TestNotificationBootstrapOffline|TestNotificationInitOfflineBranch|TestSetupCodexE2EConfigureNotifications'