Uh oh!
There was an error while loading. Please reload this page.
fix: reconcile command-less serve shapes in applyManifest - #10
Open
m4ttheweric wants to merge 1 commit into
Open
fix: reconcile command-less serve shapes in applyManifest#10m4ttheweric wants to merge 1 commit into
m4ttheweric wants to merge 1 commit into
Conversation
Previously applyManifest only synced an existing record when the resolved shape had a start command, so a port-only app activating a port-only alt returned success while keeping its base port/route, and a supervised app that dropped commands.start silently kept its old service. Now the existing-record branch handles every resolved shape: - port-only (external) app whose port changed -> editApp propagates the new port + route alias (editApp's external path updates the alias, no launchd). - external -> service (start added) -> 400 refusal: editApp keeps a record's kind, so it would set a command but never install launchd. Refuse rather than half-apply; `deck remove` + re-register is the path. - service -> route-only (start dropped) -> 400 refusal: tearing a live service down to route-only is a structural change editApp does not do. Adds 3 tests: port-only alt re-points the route (asserts edge.aliases), and both kind transitions are refused with state left unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Warning Review limit reachedNext included review available in 49 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 74 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
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 freeto 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.
Follow-up to #7, closing the deferred CodeRabbit finding on
applyManifest.Problem
applyManifestonly reconciled an existing record's serve shape when the resolved shape carried a start command (else if (shape.command)). Two consequences:deck altwas a silent no-op for routing.commands.startsilently kept its old launchd service.Fix
The existing-record branch now handles every resolved shape:
editApp({ port })propagates the new port + route alias.editApp's external path updates the portless alias without touching launchd, sodeck altactually re-routes.editAppkeeps a record'skind, so it would set a command but never install launchd, leaving a route-only app with a command and no service. Refuse rather than half-apply;deck remove+ re-register is the explicit path.editAppdoesn't perform; refusing beats silently keeping the stale service.A full kind-transition capability in
editApp(both directions, in place) is deliberately out of scope ... the refusals turn a silent wrong-state into a clear, actionable error, which is the actual defect.Behavior note
External-app port changes now flow through
editAppon every alt toggle / manifest edit, where they previously no-opped. External /staticPortports are exempt from deck's port-collision check by design (they're the user's own process, not deck's to arbitrate), so two external apps can be aliased to the same port with no 409. This is consistent with the existing stance for that field, not a new arbitration; calling it out since the pathway is now reachable more often.Testing
bun test core src= 515 pass / 0 fail (512 baseline + 3 new).kind/port/commandand theedge.aliasesroute map), not just status codes: port-only alt re-points the route; external→service refused with kind unchanged; service→route-only refused with the service kept.🤖 Generated with Claude Code