fix: repair authored skill installation and verification safety - #20
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. |
![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: 885bbca76e
ℹ️ 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".
| lock = target.parent / ("." + target.name + ".install-lock") | ||
| lock.mkdir() # exclusive; never remove another installer's lock |
There was a problem hiding this comment.
If the process is killed or the machine loses power after lock.mkdir() but before the finally block, the empty lock directory remains permanently. Every subsequent installation then fails with FileExistsError, even when the target itself was never modified, and neither the error nor the upgrade documentation provides a recovery path. Record ownership in the lock and reclaim a demonstrably stale lock, or emit actionable stale-lock recovery instructions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 9c7a0bd52e1ba9721a55037ab864761d062096e5; equivalent fixes are applied across the six creative variants and both Neon hub helpers where applicable.
Stale-lock errors and upgrade documentation now identify the exact lock and describe manual fail-closed inspection/recovery. No automatic unsafe reclamation was added.
Independent scoped review plus bounded parent rereview passed. Parent reran broad creative suites before the final one-line optional Unicode handling correction; final adversarial suite: 168 passed, with real valid/invalid hub payload checks. Existing license grants remain unchanged. No merge or live-profile installation.
| if target.exists(): | ||
| backups.mkdir(parents=True, exist_ok=True) | ||
| backup = backups / uuid.uuid4().hex | ||
| shutil.copytree(target, backup, symlinks=True) |
There was a problem hiding this comment.
When copying an existing installation is interrupted or fails partway through—for example because the disk fills or an output contains an unsupported special file—copytree leaves a partial UUID directory in the canonical backup namespace. That directory is indistinguishable from a completed backup and can later be selected for recovery even though files are missing. Copy into a temporary/incomplete location and rename it into backups only after the copy succeeds, or remove the partial tree on failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 9c7a0bd52e1ba9721a55037ab864761d062096e5; equivalent fixes are applied across the six creative variants and both Neon hub helpers where applicable.
Backups publish only after copying and writing a completion/target marker; rollback skips incomplete or malformed entries.
Independent scoped review plus bounded parent rereview passed. Parent reran broad creative suites before the final one-line optional Unicode handling correction; final adversarial suite: 168 passed, with real valid/invalid hub payload checks. Existing license grants remain unchanged. No merge or live-profile installation.
| "repository": git("remote", "get-url", "origin"), | ||
| "source_commit": git("rev-parse", "HEAD"), | ||
| "source_dirty": bool(git("status", "--porcelain")), |
There was a problem hiding this comment.
For a copied or extracted skill directory located anywhere inside an unrelated Git worktree, git -C <source> searches parent directories and records that enclosing repository's HEAD and dirty state in the installation receipt. Conversely, outside a worktree a failed status command becomes source_dirty: false. These common archive/copied-source cases therefore produce false provenance rather than an explicit unknown value; verify the discovered worktree corresponds to the distribution source and preserve Git lookup failures as unknown.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 9c7a0bd52e1ba9721a55037ab864761d062096e5; equivalent fixes are applied across the six creative variants and both Neon hub helpers where applicable.
Unavailable Git/dirty state is null, not false; enclosing unrelated repositories are not claimed as source provenance. Legitimate tracked Neon hubs retain explicit root/subtree provenance; invalid optional metadata degrades to null without bypassing payload validation.
Independent scoped review plus bounded parent rereview passed. Parent reran broad creative suites before the final one-line optional Unicode handling correction; final adversarial suite: 168 passed, with real valid/invalid hub payload checks. Existing license grants remain unchanged. No merge or live-profile installation.
* feat: sync public mirror to 3.27.0 skill surface Bring VERSION, SKILL, schemas, manifest, CHANGELOG, and ROADMAP in line with org 3.27.0: required logic_label/stage/evidence_ledger, Gate ISO, and PaperPilot PILOT. Keep #20 installer/verification safety. Co-authored-by: Daniel Meyer <scrimshawlife@gmail.com> * fix: emit logic_label from zero-option-executable recipe 3.27 requires logic_label on zero-option packets. Align the wave3 opportunity fixture and distribution-spine Procedure heading so CI matches the v0.21 skill body. Co-authored-by: Daniel Meyer <scrimshawlife@gmail.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
1 participant
Summary
Repair authored skill installation and upgrade safety: active-profile routing, validated staged activation, output/session preservation, explicit failure status and recoverable interrupted renames. Correct affected recipes, packaging and current-distribution metadata; preserve existing license grants and divergent variants.
Verification
PYTHONPATH=src:scripts python -m pytest -q -ra --tb=short -o addopts= tests_audit scriptsin an isolated Python 3.12 environment.Limits
No real profile installation, live provider usage, merge or release. Upgrade transactions are not power-loss/crash-atomic; stop runtime writers while upgrading. Existing customizations outside preserved output are retained in backup rather than automatically merged.