[jdodson]

Summary

WHY:

When config:edit is interrupted while an external editor is open, the temporary file can remain on disk with config values in it.

WHAT:

• Clean up the temporary config file on editor errors, SIGINT, SIGTERM, SIGHUP, and process exit while preserving existing signal behavior.
• Prevent failed or interrupted edits from continuing to confirmation or updating config.
• Add unit and subprocess integration coverage for cleanup, signal handling, and terminal closure.

Type of Change

Breaking Changes (major semver update)

• Add a ! after your change type to denote a change that breaks current behavior

Feature Additions (minor semver update)

• feat: Introduces a new feature to the codebase

Patch Updates (patch semver update)

• fix: Bug fix
• deps: Dependency upgrade
• revert: Revert a previous commit
• chore: Change that does not affect production code
• refactor: Refactoring existing code without changing behavior
• test: Add/update/remove tests

Testing

Notes:

Added tests across three files to verify this was fixed:

• test/unit/lib/config/util.unit.test.ts: covers successful edits, temp-file acquisition/write/read failures, editor spawn errors and exit statuses, SIGINT/SIGTERM/SIGHUP interruption, cleanup failures, listener cleanup, concurrent edits, and process-exit cleanup.
• test/unit/commands/config/edit.unit.test.ts: verifies editor failures in both full-config and single-key modes skip confirmation and config updates.
• test/integration/config/edit.integration.test.ts and test/integration/config/edit-command.integration.test.ts: subprocess tests verify tempfile removal on SIGINT, SIGTERM, and SIGHUP and process exit. The CLI-level tests also cover parent-only signals and macOS PTY closure, checking the file is gone while the editor is still alive and config was not updated.

Screenshots (if applicable)

Related Issues

GUS work item: https://gus.lightning.force.com/lightning/r/ADM_Work__c/a07EE00001vk5ojYAA/view

[github-actions]

This PR is labeled size/XL (2067 lines of code churn, excluding docs, package-lock.json, and generated files).

Large PRs are harder to review well. If it can be split into smaller, independently reviewable pieces, consider doing so. This is a suggestion, not a blocker — the check still passes.

[eablack]

Approving — both blocking issues resolved

Thanks for the thorough rework. Re-tracing the paths, both blocking issues are genuinely fixed and each is now pinned by a dedicated, precise test:

1. Successful edit no longer turns into a failure on cleanup error. A failed unlink on the success path now only reports via the synchronous writeSync(2, …) warning and still returns result — it never sets editError. Verified by "returns edited content and warns when temporary-file cleanup fails", which asserts result === edited, the file still exists, and the diagnostic is exactly Failed to clean up temporary config file. That exact-message assertion also tightens the loose cleanup-failure coverage I'd flagged.

2. The editor child is killed (and awaited) before its temp file is unlinked. terminateChild() sends SIGTERM, awaits exit, escalates to SIGKILL, and cleanup() runs only after the child has terminated. "kills and awaits the spawned editor before removing its temporary file on interruption" nails the contract: kill called with SIGTERM, temp file must remain while the editor is still running, the edit must await termination, and the file is removed only after the child exits.

Also addressed from the non-blocking list: the dead reentrancy/interruptedSignal guards are gone, the fd-2 write has a justifying comment, the shebang now uses process.execPath, and the concurrency test asserts cleanup-before-reraise ordering. Extracting the signal handling into a general process-shutdown.ts coordinator is a reasonable answer to the architecture/telemetry-coupling concern — not the minimal tmp.setGracefulCleanup() shape I floated, but a legitimate design and no longer a leaf util with leaking module state.

CI is green across all platforms, including the macOS jobs.

One non-blocking follow-up (not a blocker): the PTY/terminal-closure coverage in edit-command.integration.test.ts:586 is still darwin-gated, and the integration CI job runs ubuntu-only, so that SIGHUP-from-PTY-close path still doesn't execute in CI. The unit suite covers the equivalent logic, so I'm fine landing as-is — worth a quick look at loosening the gate to linux+darwin in a follow-up if it's cheap.