fix(bin): make Lavish delivery ACKs capture-safe - #86
Merged
Merged
Conversation
…egression coverage
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Make bin/fm-procevent-lavish.sh delivery-ACK-aware as the pre-install integration gate for the patched Lavish durability change at local commit 8ba3f32. The full poll result must be durably captured in the existing procevent inbox before acknowledging its delivery_id through the exact patched Lavish CLI, and terminal retirement must occur only after acknowledgement is confirmed; capture, ordering, truncation, or ACK-command failure must keep the source armed for safe redelivery. Preserve compatibility with unpatched lavish-axi responses that have no delivery_id: issue no ACK and keep current behavior. Replace the adapter LOSS LIMITATION contract with an accurate conditional guarantee, explicitly recording that delivery has no owner or TTL and is at-least-once, duplicate-tolerant, and non-exclusive, and that Lavish state uses plain writeFile rather than fsync plus atomic rename. Cover capture-before-ACK ordering, ACK failure, final session_ended feedback ACK-before-retirement, and no-delivery_id compatibility with a protocol-faithful fake lavish-axi, plus one opt-in isolated end-to-end capture, ACK, and redelivery test built from exact commit 8ba3f32 in scratch on an ephemeral port. Never invoke the globally installed lavish-axi or the protected shared server at 127.0.0.1:4387, never install or upgrade Lavish, and never modify or push the read-only Lavish checkout. Keep changes to bin/fm-procevent.sh minimal and adapter-agnostic; that post-capture hook is necessary because the registered Lavish listener cannot itself acknowledge only after the external generic runner has durably captured its output. Follow firstmate shared-tracked-material rules, keep tests colocated, and remain ShellCheck-clean.
What Changed
delivery_idretain legacy behavior.writeFilepersistence caveat.8ba3f32on an ephemeral port and verifies capture, ACK, and redelivery.Risk Assessment
🚨 High: Captain, a normal bearings-board delivery can remain permanently unacknowledgeable, while legacy behavior and authoritative durability and retirement contracts remain inconsistent, so this is not safe to merge without resolving the source paths.
Testing
The fake protocol suite covered capture-before-ACK, feed and ACK failure, truncation, final-session retirement, and legacy responses; the existing generic runner suite also completed. The real opt-in E2E passed from exact Lavish commit 8ba3f325... in a disposable scratch clone using an ephemeral port, proving redelivery until capture and ACK. The default read-only checkout was newer and correctly failed closed, so it was not modified. No global lavish-axi, shared port 4387, linter, formatter, install, or upgrade was used.
Evidence: Lavish ACK validation transcript
Source: Lavish ACK validation transcript
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-procevent-lavish.sh:318- The ACK call also performs a follow-on poll whose output is discarded. If final delivery A is ACKed while newer delivery B is queued, the patched Lavish CLI returns B successfully; this function returns success, then the generic runner retires the source based on A. B remains unacknowledged but no listener remains to redeliver it. Preserve the follow-on result or keep the source armed whenever a newer delivery is observed.bin/fm-procevent-lavish.sh:99- The required criterion is: “Replace the adapter LOSS LIMITATION contract with an accurate conditional guarantee.” Although the new adapter block does this, authoritative owners still state that all Lavish polls destructively clear feedback and must never be described as at-least-once (docs/configuration.md:654-655,.agents/skills/process-event-sources/SKILL.md:109-112, anddocs/verification/process-event-sources.md:69-84). Update those existing contracts conditionally for patched delivery_id responses versus legacy responses without IDs.tests/fm-procevent-lavish-live-e2e.test.sh:19- The required criterion is: “one opt-in isolated end-to-end capture, ACK, and redelivery test built from exact commit 8ba3f32.”FM_LAVISH_PATCH_COMMITcan replace8ba3f32, and the test never asserts the expected commit hash, so an opt-in run can validate a different checkout while presenting the same proof. Pin or assert the exact expected commit while retaining only the checkout-path override.🔧 Fix: Updated conditional Lavish contracts and pinned exact E2E commit
1 error still open:
bin/fm-procevent.sh:489- The required criterion is: “The full poll result must be durably captured in the existing procevent inbox before acknowledging its delivery_id,” and capture failures must keep the source armed. The new ACK hunk atbin/fm-procevent.sh:489-493ACKs every non-truncated, nonempty capture, while the earlier guard rejects a nonzero poll exit only when output is empty (:475-481). A poll that emits a validdelivery_idprefix and then exits nonzero can therefore be captured and ACKed despite being incomplete, losing the delivery without an error. Require successful poll completion or an explicit complete-result marker before ACKing.🔧 Fix: Gated ACK on successful poll completion; added regression coverage
1 warning still open:
tests/fm-procevent-lavish-ack.test.sh:94- Theack-failurescenario exercises a non-terminal feedback response, so the source would remain registered even if the new terminal-retirement ACK gate were broken. It therefore does not prove the required terminal ACK-failure sequence. Make this scenario emitsession_endedor add a separate terminal ACK-failure case and assert that the source remains registered.🔧 Fix: Made ACK-failure regression terminal and retirement-sensitive
3 issues (2 errors, 1 warning) still open:
bin/fm-procevent.sh:489- A bound Lavish result can be ACKed before the keyed-answer feed; if that feed fails or the runner dies, reconciliation only republishes the result and never retries the feed, so the captain-held answer remains unrecorded.bin/fm-procevent.sh:490- For a poll that is both truncated and nonzero, the truncation branch suppresses the exit-code branch, emitting two status lines while omitting the poll exit code required for partial failures.tests/fm-procevent-lavish-ack.test.sh:80- The truncation test does not check start success or durable result creation, so an early failure after polling can pass. Assert successful start and the expected truncated result.🔧 Fix: Gate keyed feeds before Lavish ACK
5 issues (4 errors, 1 warning) still open:
bin/fm-procevent.sh:506- The new gate atbin/fm-procevent.sh:506-510treats any nonzero keyed-answer intake as a reason not to ACK. The bearings board is bound with any-origin (bin/fm-bearings-board.sh:184-187), but its documentedmerge.<task-id>anddispatch.chartedchoices are intentionally routed by firstmate after the wake (.agents/skills/bearings/SKILL.md:88,104-109).fm-procevent-lavish.sh answersemits those keys, whilefm-captain-hold.sh answersskips them and exits nonzero (bin/fm-captain-hold.sh:707-710,766-767), so normal board deliveries never reach the ACK at:523and remain repeatedly unacknowledged. Filter non-captain routing keys at the adapter boundary or provide a durable pending route before ACK.bin/fm-procevent-lib.sh:23-bin/fm-procevent-lib.sh:22-30still states thatlavish-axi polldestructively clears every response and says “Never describe this runner as at-least-once.” The changed adapter contract atbin/fm-procevent-lavish.sh:99-111and the required intent instead require a conditional at-least-once guarantee for responses withdelivery_id, with only the no-ID legacy path remaining destructive. This shared durability contract is still contradictory; update it with the delivery_id/no-ID split and the required no-owner/TTL and plain-writeFile limits.docs/configuration.md:610- The required criterion is “terminal retirement must occur only after acknowledgement is confirmed,” butdocs/configuration.md:610still says the runner retires on a terminal exit 0 alone, and.agents/skills/process-event-sources/SKILL.md:90says retirement is independent of acknowledgement. The changed runner now requiressource_acknowledged == 1atbin/fm-procevent.sh:553, so ACK, truncation, or bound-feed failures leave terminal Lavish registrations armed. Update these authoritative contracts to describe the ACK-aware retirement rule.bin/fm-procevent.sh:498- The intent requires “Preserve compatibility with unpatched lavish-axi responses that have no delivery_id: issue no ACK and keep current behavior.”source_ack_requiredis enabled for the entire Lavish adapter atbin/fm-procevent.sh:498-500, so the new feed/truncation/exit gate at:506-522also changes no-ID results. A no-ID terminal result from a bound source whose keyed feed fails is captured but neither externally ACKed nor retired at:553, whereas the base path retired adapter-terminal results; clean no-ID results are also labeledsource-acknowledgedeven thoughbin/fm-procevent-lavish.sh:309-310performs no ACK. Split the per-result no-ID path from the delivery-ACK gate so legacy lifecycle and output semantics remain unchanged.tests/fm-procevent-lavish-ack.test.sh:39- The added fake is described as a “protocol-faithful fake lavish-axi,” but its successful ACK branch only logs and returns (tests/fm-procevent-lavish-ack.test.sh:29-49); it never records the acknowledged delivery, and later normal polls always emit the same fixeddelivery_id(:70-80). A regression where ACK returns success but the delivery remains queued would therefore pass the synthetic suite; the opt-in real E2E may be skipped. Model the ACK state and verify the subsequent poll no longer returns that delivery.✅ **Test** - passed
✅ No issues found.
bash tests/fm-procevent-lavish-ack.test.shbash tests/fm-procevent.test.shgit -C /Users/ivan/Projects/firstmate/projects/lavish-axi rev-parse --verify 8ba3f32^{commit}FM_LAVISH_LIVE_E2E=1 FM_LAVISH_PATCH_SOURCE=<disposable exact-commit scratch clone> bash tests/fm-procevent-lavish-live-e2e.test.shgit status --short --branch✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.