Skip to content

[codex] Refactor patch registry descriptors#167

Merged
ilysenko merged 1 commit into
mainfrom
codex/refactor-patch-registry
May 12, 2026
Merged

[codex] Refactor patch registry descriptors#167
ilysenko merged 1 commit into
mainfrom
codex/refactor-patch-registry

Conversation

@ilysenko
Copy link
Copy Markdown
Owner

Summary

Refactors the shipped Linux ASAR patch registry into auto-discovered patch descriptors under scripts/patches/core/**/patch.js, with descriptors grouped by target namespace (all-linux, distro, package, desktop). Adds a shared descriptor engine, Linux target detection helpers, and structured patch-report target metadata so future distro/package/desktop-specific patches can self-filter without touching the rest of the registry.

Also fixes the Nix fixed-output payload path after the update-builder contents changed. The payload now prunes nondeterministic native-module build artifacts, sets deterministic build/remap flags, refreshes the pinned recursive outputHash, and keeps the Nix installer shellcheck-clean.

Validation

  • node --test scripts/patch-linux-window-ui.test.js
  • node --test linux-features/*/test.js
  • bash -n scripts/lib/native-modules.sh
  • bash tests/scripts_smoke.sh
  • git diff --check
  • Docker Nix: nix build .#codex-desktop .#installer --no-link --print-build-logs --option sandbox false

Notes

The two-hour update-codex-hash.yml workflow runs scripts/ci/update-nix-hashes.sh against .#codex-desktop; that path now accepts the refreshed fixed-output payload hash in a clean Nix container.

@ilysenko ilysenko added the autoreview/draft Bot skipped: PR is in draft state label May 12, 2026
@ilysenko ilysenko marked this pull request as ready for review May 12, 2026 13:22
@ilysenko ilysenko added autoreview/needs-human Bot needs a human to review this issue autoreview/risk-high Bot judged risk as high; human review required and removed autoreview/draft Bot skipped: PR is in draft state labels May 12, 2026
Copy link
Copy Markdown
Owner Author

@ilysenko ilysenko left a comment

Choose a reason for hiding this comment

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

Review Mate: merge not recommended yet

🔴 Recommendation: merge not recommended yet
Risk level: high

Do not merge until the flagged risks are addressed.

This PR restructures the Linux ASAR patch registry into auto-discovered descriptor files and also carries a Nix fixed-output payload/hash cleanup in the same change set.

What changed

The patcher source of truth moves from a central registry model toward scripts/patches/core/**/patch.js descriptors driven by a new scripts/patches/engine.js, with scripts/patches/registry.js becoming orchestration. The PR also changes the Nix installer path in flake.nix and references changes to native-module payload normalization.

Why it matters

This is in the repo’s most fragile area: patch ordering, CI policy, target filtering, and patch-report metadata. Even if the descriptor engine is functionally equivalent, this is a broad patcher refactor rather than a narrow bug fix, and the PR also mixes in an unrelated Nix payload fix. That combination puts it above the auto-merge bar for this project.

Notes for the reviewer

I could not run local git diff inspection because the read-only command sandbox fails before git starts with the host’s disabled unprivileged user namespace setup. The risk call is therefore based on the supplied PR metadata and visible diff excerpt, not a full local line-by-line audit.

Risks

  • High-risk patcher refactor: the PR changes the patch registry architecture and introduces auto-discovered core descriptors under scripts/patches/core/**/patch.js, which directly affects patch order, CI policy, and patch-report behavior in the hot patcher area.
  • Scope-creep risk: the PR combines the patch registry descriptor refactor with a separate Nix fixed-output payload/path fix, which matches the maintainer’s recurring rejection pattern for unrelated refactor plus fix in one PR.
  • Needs human validation that every moved descriptor preserves previous ordering, required-upstream policy, idempotency behavior, and smoke-fixture expectations across main-process, webview, Computer Use, keybinds, launch-actions, and package-json patches.
  • Local full-diff verification was blocked by the execution sandbox, so caller checks, generated/source-of-truth checks, and exact smoke-test alignment could not be independently confirmed.

References

  • AGENTS.md:56
  • AGENTS.md:57
  • AGENTS.md:58
  • AGENTS.md:144
  • AGENTS.md:194
  • flake.nix:86
  • scripts/patches/registry.js:1
  • scripts/patches/engine.js:1
  • scripts/patches/core/all-linux/patch.js:1
Second opinion (Claude)

Risk level: high

This PR converts the shipped Linux ASAR patch registry from explicit lists in scripts/patches/registry.js to filesystem-discovered descriptors under scripts/patches/core/**/patch.js, grouped by target namespace (all-linux, distro, package, desktop). It introduces scripts/patches/engine.js (descriptor discovery, normalization, duplicate-id check, target-filter dispatch, structured patch-report metadata) and a new scripts/lib/linux-target-context.js for distro/package/desktop matching. The same PR also rewrites the Nix codexDesktopPayload install path: it prunes native-module build artifacts, sets deterministic build/remap flags (SOURCE_DATE_EPOCH, -ffile-prefix-map=..., --remap-path-prefix=..., -Wl,--build-id=none), and bumps the pinned recursive outputHash.

What changed

  • scripts/patches/registry.js — orchestrates discovered core descriptors via engine.js; legacy export surface (MAIN_BUNDLE_PATCHES, WEBVIEW_ASSET_PATCHES, COMPUTER_USE_UI_ASSET_PATCHES, corePatchDescriptors, requiredPatchNamesForProfile) preserved.
  • scripts/patches/engine.js — new descriptor engine (discover / normalize / sort / dispatch by phase).
  • scripts/patches/core/all-linux/{main-process,webview,extracted-app}/*/patch.js — twelve new descriptor files, each delegating to the existing apply* functions in main-process.js, webview-assets.js, computer-use.js, etc.
  • scripts/lib/linux-target-context.js — new target-detection helpers (matchesId, packageFormatIs, desktopMatches, versionAtLeast).
  • scripts/patch-linux-window-ui.test.js, tests/scripts_smoke.sh — tests extended for descriptor discovery, duplicate-id rejection, target-filter skip, and the new core/* + lib/linux-target-context.js packaged files.
  • scripts/lib/package-common.sh:179 — copies linux-target-context.js into the update-builder payload (descriptors come along via the existing cp -r scripts/patches/. line).
  • scripts/ci/validate-patch-report.js — unchanged in behavior; consumes requiredPatchNamesForProfile (now backed by descriptors).
  • flake.nixcodexDesktopPayload install phase adds prune + deterministic flags; outputHash refreshed (sha256-efRH4r4KIIsvAxMyzy2sd+Cezv1N5zKKEWyz+6vMc5Y=); two # shellcheck disable=SC2016 comments added; new patchNixGeneratedScripts helper used inside the payload.
  • AGENTS.md — updated to point at scripts/patches/core/**/patch.js as the new source of truth for shipped Linux patches.

Why it matters

This touches the project's most refactor-hostile area. The maintainer's standing position from AGENTS.md / past PR history is that hot patchers (scripts/patches/*.js) should receive bug-fix-only changes — refactors that wrap fixes in generator patterns have been rejected. This PR is a pure refactor, but it (a) restructures the source-of-truth boundary for shipped patches (now scripts/patches/core/**/patch.js rather than registry.js), (b) touches all currently shipped patcher entrypoints simultaneously by re-homing them into descriptor files, and (c) bundles an unrelated Nix payload concern (artifact pruning + deterministic flags + recursive hash refresh) into the same diff. Any of these warrants its own review pass; together they substantially raise the diff's blast radius across all five packaging paths.

The descriptor model itself looks well-formed — engine.js enforces unique ids, sorts by (order, sourcePath, id), threads appliesTo(context) for distro/package/desktop self-filtering, and records skipped-target for filtered patches with targetSummary metadata. The main-process-ui aggregate report entry is preserved so existing CI consumers don't break. Re-exports through patch-linux-window-ui.js keep the old test/CLI surface working. Smoke and node tests are updated for the new layout. The behavioral risk is mostly schema- and ordering-stability around patch-report.json (new linuxTarget, new per-patch phase / targetSummary fields) and around the new fail-fast assertUniquePatchIds (a future patch with a duplicate id now throws instead of warning).

Notes for the reviewer

  • The Nix payload changes (flake.nix:182-269) are conceptually independent of the registry refactor. Consider asking the author to split into two PRs — registry refactor vs. payload determinism — to make the patcher half easier to validate and the Nix half easier to revert if it breaks nix build in CI.
  • The recursive payload outputHash was refreshed (flake.nix:205); the upstream codexDmg hash on flake.nix:25 was not touched in this PR but should still validate because the DMG URL is unchanged. There are no separate pinned Cargo / Node / Electron hashes in this flake (it uses __structuredAttrs + outputHashMode = recursive), so the single hash refresh appears sufficient — worth confirming the maintainer's hash-refresh checklist matches.
  • discoverCorePatchDescriptors walks the filesystem at every call site (e.g. mainBundlePatchDescriptors, patchExtractedApp calls it twice, plus EXPORTED_CORE_PATCHES at module load). Not a correctness issue, but it means a missing/broken descriptor will throw at registry require-time rather than at apply-time — verify upstream regenerate flows can tolerate that.
  • legacyCorePatchDescriptors is exported but only delegates to corePatchDescriptors — looks like a transitional shim with no current external caller; safe to drop in a follow-up.
  • tests/scripts_smoke.sh:154-156 asserts existence of core/distro/nixos/README.md, core/desktop/i3/README.md, core/package/deb/README.md — confirm these placeholder READMEs are present in the diff (the empty namespaces are the contract surface for future distro-specific patches).

Risks

  • Hot patcher refactor risk: this restructures scripts/patches/registry.js and re-homes every shipped patch into scripts/patches/core/**/patch.js. Past project memory (and AGENTS.md) flags refactors of this area as the most common rejection reason — anchored expressions in main-process.js / webview-assets.js are kept unchanged but the surrounding orchestration is rewritten, increasing rebase friction with upstream syncs.
  • Scope creep: the PR bundles a registry refactor with an unrelated Nix payload restructure (artifact pruning, deterministic compile flags, outputHash refresh) in flake.nix:211-269. Per project convention, mixed-scope PRs are routinely split or bounced — these two changes have independent failure modes and should be reviewable independently.
  • Source-of-truth boundary shift: AGENTS.md is updated to say shipped Linux patches now live under scripts/patches/core/, while registry.js keeps re-exporting the legacy constants. Future contributors may be confused about where to add new core patches if both surfaces remain exported.
  • New strict invariant: engine.js::assertUniquePatchIds now throws on duplicate ids (scripts/patches/engine.js:90). This is stricter than the pre-refactor behavior; if a feature/* or future descriptor inadvertently reuses an id, the entire patch run now hard-fails instead of warning.
  • patch-report.json schema additions (linuxTarget object on the report, per-patch phase + targetSummary) may surprise consumers that pin fields. Smoke and validate-patch-report.js are unaffected because they read by name/status, but downstream rebuild-report consumers should be checked.
  • Filesystem discovery walks scripts/patches/core/ synchronously on require (registry.js:224 EXPORTED_CORE_PATCHES = corePatchDescriptors()) and at every patch invocation. Errors in any descriptor file now manifest at module load time, which can mask the actual cause behind a generic require() throw.
  • Nix outputHash refresh (flake.nix:205) was generated against the new pruned/deterministic install path; if the prune step or any of the new compile flags differ between CI sandbox modes, the hash will drift and nix build will fail reproducibly elsewhere.

🤖 Reviewed by codex + claude.

@ilysenko ilysenko merged commit b951b2a into main May 12, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autoreview/needs-human Bot needs a human to review this issue autoreview/risk-high Bot judged risk as high; human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant