feat: ship portable mutation journal helper (v0.7.0) - #21
Conversation
TueJon
left a comment
There was a problem hiding this comment.
-
Blocking — lock ownership can disappear while the parent still authorizes a mutation.
skills/webmcpify/templates/mutation-journal.ts:351checks#holderonly once, before the firstawait; all reads, replacement/fsync work, and the return that authorizes the caller to dispatch happen afterward. I opened a journal, sentSIGKILLto itsflockholder, and immediately calledbeforeDispatch; it returned an execution ID and durably appendedstartedafter the kernel lock was gone. A successor can therefore acquire the sidecar while the first runner continues intoexecuteTool(webmcp.spec.ts:309). The same unfenced window exists in startup/migration (:329-340), settlement (:409-437), and release (:446-464; an exit between the initial check and listener registration can also leaveclose()waiting for an event already emitted).Closure checklist:
- Make startup/migration,
beforeDispatch,settle, and final release share an ownership mechanism whose loss fences every manifest write and dispatch authorization. - Ensure
beforeDispatchcannot return authorization after ownership loss, including loss at each asynchronous read/write/fsync boundary used by the Playwright template. - Make
close()handle holder exit at every point without a missed-event hang. - Add killed-holder/concurrent-successor regressions for startup, dispatch, settlement, and release, and keep the TS/JS twins in sync.
- Make startup/migration,
-
Blocking — malformed journal data is silently erased or ignored, so uncertain mutations fail open.
skills/webmcpify/templates/mutation-journal.ts:177-195treats every non-arraymutationExecutionsvalue as a legacy absence and replaces it with[]; arrays are scanned by filtering only exactstate === 'started', so malformed/unknown entries are likewise treated as safe. Reproduction: a v4 tool containingmutationExecutions: { executionId: "uncertain", state: "started" }opened successfully, reportedunresolved: [], and rewrote the value to an empty array. That violates the documented “never erase existing journal entries” boundary and permits a new mutation when the durable safety record is corrupt.Closure checklist:
- Migrate only a genuinely absent legacy field; reject present non-array values without rewriting the manifest.
- Validate every journal entry and state before unresolved scanning or any write; unknown/malformed states must fail closed rather than disappear from the scan.
- Reject ambiguous identity/linkage such as duplicate execution IDs or invalid parent ownership before
beforeDispatch/settlecan select an entry. - Add preservation/fail-closed regressions for non-array journals, malformed entries, unknown states, duplicates, and bad parent links in both helper variants.
-
Blocking — a repository-controlled
manifest.locksymlink can overwrite an arbitrary writable file.skills/webmcpify/templates/mutation-journal.ts:322-338creates/opens the sidecar by pathname, thenwriteOwnerMetadatareopens that pathname and truncates it at:199-204. I made.webmcpify/manifest.locka symlink to a victim file;openMutationJournal()replaced the victim contents with owner metadata. Path re-resolution also means the file checked/created by the parent is not guaranteed to be the inode the lock utility owns.Closure checklist:
- Reject symlink, non-regular, and multiply-linked sidecars without touching their targets.
- Bind creation, advisory locking, metadata writes, and identity checks to the same verified inode without an unchecked pathname reopen/swap window.
- Add regressions proving a symlink target remains byte-identical and a replaced sidecar cannot create two lock identities; mirror the fix in TS and JS.
-
Blocking — this exact head conflicts with current
mainat the self-hosted-runner safety gate..github/workflows/ci.yml:13has the new self-hosted label but lacks currentmain's fork-PR guard (if: github.event_name == 'push' || ...head.repo.full_name == github.repository). GitHub reports this headCONFLICTING, and the PR also states that it must be rebased after #20 before the 0.7.0 release metadata is final.Closure checklist:
- Rebase after the prerequisite release head lands and resolve the CI conflict while retaining
main's fork guard. - Reconcile changelog/version/release ancestry so 0.7.0 follows the final 0.6.1 head.
- Re-run the exact-head manifest/version and CI checks after that integration.
- Rebase after the prerequisite release head lands and resolve the CI conflict while retaining
npm run check passes all 54 tests on this head; the three focused reproductions above exercise missing failure boundaries rather than duplicating that suite.
# Conflicts: # .claude-plugin/marketplace.json # .claude-plugin/plugin.json # .cursor-plugin/plugin.json # .github/workflows/ci.yml # CHANGELOG.md # gemini-extension.json # package-lock.json # package.json # skill.json
|
Finding 1 — fixed in e2aab82. The runner process now owns the advisory lock through its retained descriptor; there is no separately killable holder process, so ownership cannot disappear while the runner continues toward dispatch. Startup, |
|
Finding 2 — fixed in e2aab82. Migration now applies only when |
|
Finding 3 — fixed in e2aab82. Sidecars are opened with no-follow semantics, must be regular and singly linked, and are bound to the retained locked descriptor. Metadata is written through that descriptor; acquisition and live-use identity checks reject pathname swaps. Regressions prove symlink and hard-link victims remain byte-identical and that acquisition-time or live sidecar replacement cannot create a second operational journal. Verified on final ancestry |
|
Finding 4 — fixed in e5bc0bd. PR #20 is merged and current |
TueJon
left a comment
There was a problem hiding this comment.
Follow-up review: all four prior closure checklists are satisfied at this head; no regressions were found in ownership/dispatch fencing, journal validation, sidecar identity, or the v0.6.1 ancestry and CI integration.
Exact-head CI (npm run check, 69 tests) passed. No blocking findings.
Summary
flock/lockf -klockingVerification
npm run check(54 tests)node --test tests/manifest-lock.test.mjs(11 tests)git diff --checkIntegration note
PR #20 is still open and reserves v0.6.1. This branch is intentionally based on current
main; it will be rebased after #20 lands, as required by #17.Closes #17