fix(cli): preserve concurrent scaffold config creation - #3721
Conversation
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed at exact head a056db1c1200feee0a439547ff39e00e0acbd18d.
The extraction at packages/cli/src/utils/writeNewFile.ts:1-18 preserves #3720’s Windows-verified protocol: private same-directory staging, complete write, atomic hard-link publication, EEXIST-only collision ownership, and finally cleanup. Both new callers create their parent directory first, and the existing package template tests retain the exact scripts/formatting contract. The 237 initialization/capture tests pass locally.
Blocker — the same create-if-absent contract still has four unsafe hyperframes.json callers. The new generic helper now exists, but these paths continue to guard the overwriting writeProjectConfig with an absence observation:
packages/cli/src/commands/init.ts:590-603packages/cli/src/commands/add.ts:278-283packages/cli/src/commands/add.ts:497-503packages/cli/src/utils/projectConfig.ts:257-263(read returns ENOENT, then a later direct write)
They have the same concurrent-winner and dangling-symlink preconditions as package/meta creation. I reproduced the symlink case through the real writer at this head: existsSync(hyperframes.json) returned false for a dangling link, writeProjectConfig preserved the symlink but created its missing target with 318 bytes. A concurrent config can likewise be overwritten between the check/read and line 187’s plain writeFileSync.
Please add a create-only project-config path backed by writeNewFileSync and route all four absent-only callers through it, while keeping the existing overwriting writer for the two deliberate update paths documented in projectConfig.ts. Add one command/add or project-config witness for concurrent content and dangling-link target non-creation. This keeps overwrite and create-if-absent as separate explicit contracts instead of changing writeProjectConfig globally.
Exact-head CI has no completed failure; JavaScript CodeQL, Windows, build/typecheck, and remaining jobs are still running.
Verdict: REQUEST CHANGES
Reasoning: The shared publication primitive is sound, but four sibling create-if-absent config paths retain the exact overwrite/symlink race this extraction is meant to centralize.
— Magi
|
Addressed the config-creation scope blocker in d78b69c. Added createProjectConfig backed by the shared staging/link helper and routed all four absent-only init/add/authoring-skill paths through it. Deliberate updates of existing configs retain their current overwrite behavior. All 303 init/add/capture/config tests pass; new skill-seeding concurrent/symlink witnesses fail with the prior writer, and command tests now cover both package.json and hyperframes.json. CLI types, lint/format, audit and signed hooks pass. Renewed review and fresh CI/CodeQL requested. |
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-review at exact head d78b69c1104402297a1ea1471b4f17f8ada2cbde. The project-config scope blocker is closed; no new blockers.
The contract split at packages/cli/src/utils/projectConfig.ts:182-197 is clean: writeProjectConfig remains the explicit overwrite API, while createProjectConfig is the absent-only API backed by the single staging/link implementation. A production caller inventory shows exactly four create-only calls—init, both add branches, and skill seeding’s ENOENT branch—and no absent-only site still reaches the overwriting writer.
seedProjectAuthoringSkill retains its best-effort boundary at projectConfig.ts:255-276: non-ENOENT read errors leave existing bytes alone; ENOENT attempts exclusive creation; a concurrent winner or dangling symlink yields the helper’s harmless EEXIST; unsupported/permission failures remain swallowed only because this caller was already explicitly best-effort. Existing-config patch writers remain separate, so unknown-key and indentation preservation are unchanged.
The new tests distinguish all three contracts rather than only checking “a file exists”:
- init command: existing/concurrent/symlink entries for both package and project config;
- project config: create-no-overwrite, deliberate overwrite, seed-after-ENOENT race, and dangling-link target non-creation;
- capture: all eight landed publication/failure-cleanup witnesses remain enabled.
Independent exact-head verification: 303/303 init/add/capture/config tests and formatting on all eight changed files passed.
Non-blocking note: both add paths load config before their absence check. If a custom config wins precisely after that read, disk now correctly preserves it but that one invocation continues with its already-loaded default. This is pre-existing transient behavior and strictly safer than overwriting the winner; it is not part of the create-publication defect.
Fresh CI has no current-head completed failure; JavaScript CodeQL, Windows, build/typecheck, and remaining jobs are still running. Older cancelled Windows runs appear as failures in the aggregate check list but are not from this head’s live run.
Verdict: APPROVE
Reasoning: Every create-if-absent caller now shares the Windows-verified publication primitive, while deliberate updates remain an explicit, separately tested overwrite contract.
— Magi
CLI scaffolding can overwrite files created between an absence check and a write, or follow a dangling symlink and create its target. This affects
package.jsonduring init and four create-if-absenthyperframes.jsonpaths in init, add, and authoring-skill seeding.Reuse capture metadata's private staging plus hard-link publication through
writeNewFileSync. A newcreateProjectConfigwrapper routes all four absent-only config paths through that helper. Only publicationEEXISTis ignored; staging is cleaned on success and failure. Existing-file shortcuts, package/config bytes, and intentional updates of existing project configs are preserved. No workflow changes.Addresses CodeQL #267 and the matching config creation races found during review.
Validation: all 303 init/add/capture/config tests pass. The two seed-creation witnesses fail with the prior writer. Command-level cases cover existing and concurrent package/config files and dangling symlinks. Project-config cases verify an ENOENT-to-create race, dangling-link target non-creation, and the deliberate overwrite contract. Existing capture publication and authoring-skill/registry update coverage remain enabled. CLI typecheck, parser/core/lint builds, lint/format and complexity audit pass. Windows CI, CodeQL and revised-head Magi approval remain merge gates.
Publication requires hard-link support on the output filesystem and propagates unsupported-filesystem or permission errors, subject to the skill-seeding caller's existing best-effort handling.