keystone/docs/implementation-review.md

Keystone Implementation Review — Gaps vs docs/implementation-spec.md

The schema/migrations/models are about 98% correct. The orchestration, drivers, UI, and tests have substantial gaps. Below are concrete, file-anchored issues to fix.

Critical orchestration bugs

1. Operation parent/child hierarchy is flat — replicas are siblings of their service_deploy, not children

Spec §3 example nests service_deploy → replica_deploy. app/Jobs/Environments/DeployEnvironment.php:74-82 creates both as siblings under the same environment_deploy parent. Replica_deploy siblings rely on RunStep::dispatchNextSiblingOperation (app/Jobs/Services/RunStep.php:120-140) ordering by id — fragile, and if service_deploy fails, replica operations are not cancelled. Fix: Nest replica operations under their service's service_deploy operation (parent_id = service_deploy.id), and cascade-cancel children when a parent fails.

2. Failed operations don't cancel siblings or children

RunStep::failed (app/Jobs/Services/RunStep.php:163-178) only cancels the failed operation's remaining steps. Sibling/child operations under the same parent continue to dispatch via dispatchNextSiblingOperation. A failing service deploy will still trigger gateway cutover. Fix: On step failure, mark parent + all descendant operations as CANCELLED/FAILED. Re-check during sibling dispatch.

3. Gateway cutover uses a hardcoded container name that doesn't exist

DeployEnvironment.php:202 runs docker exec keystone-caddy caddy reload ..., but Caddy replicas are created with name keystone-service-{service->id}-{N} per DeployEnvironment.php:155. The keystone-caddy container is never created — cutover will always fail. Fix: Look up the Caddy service's replica container name; or set a stable container_name in Caddy compose.

4. Gateway cutover is monolithic; no add-upstream/reload/drain sub-sequence

Spec §15 requires: render new replica → health check → add new upstream → reload → drain old → stop old. DeployEnvironment.php:201-206 does only caddy reload && sleep 10 && stop draining. There's no add-upstream step (Caddyfile is fully overwritten at slice_configure time), no real health check during cutover, and sleep 10 is an arbitrary drain. Fix: Split into separate steps with explicit ordering, and tie drain to active connections / Caddy upstream health.

5. dispatchChildOperations dispatches only the first child's first step

DeployEnvironment.php:377-387 dispatches a single step. Continuation depends on RunStep::dispatchNextSiblingOperation chasing siblings by id. If a child operation has zero steps (e.g. service_deploy for a service whose driver returned no plan), the chain dies silently. Fix: Make a single orchestrator (e.g. DispatchOperationChain) that knows how to walk the tree. Don't rely on implicit id-ordering between independently-created sibling operations.

Driver contract & implementations

6. Driver base contract is anemic

Spec §9 lists 13 required driver capabilities. app/Drivers/Driver.php only declares __construct and getOperationPlan. Image policy, ports, volumes, env schema, health checks, resource defaults, slice types, env exports, firewall, update behavior are scattered across drivers without contractual enforcement. Fix: Define an interface with explicit methods (type(), versionTrack(), defaultPorts(), firewallRules(), updateBehavior(), etc.) and assert per-driver via tests.

7. Caddy2Driver::buildCaddyfile() reads an undefined field — dead code

app/Drivers/Caddy/Caddy2Driver.php:46 references $this->service->credentials['backend']. Nothing ever sets this. The actual Caddyfile is rendered inline by DeployEnvironment::configureCaddyRouteScript (app/Jobs/Environments/DeployEnvironment.php:321-335). Fix: Delete buildCaddyfile(), or move Caddyfile generation into the driver and remove the duplicate in DeployEnvironment.

8. Postgres18Driver has no slice provisioning in its operation plan

Spec §12: "Creating a Postgres database/user should run as a slice operation against an existing Postgres replica, not redeploy the Postgres container." AttachManagedService::createSliceProvisionOperation (app/Actions/Environments/AttachManagedService.php:108-125) hardcodes the SQL script outside the driver. Slice provisioning logic belongs in the driver so other Postgres versions can implement it. Fix: Add provisionSliceScript(ServiceSlice $slice): string to the slice contract; have Postgres driver own the SQL.

9. Postgres provision script assumes a keystone admin user that is never created

AttachManagedService.php:133 uses ($service->credentials ?? [])['user'] ?? 'keystone'. The Postgres service is never seeded with admin credentials and the compose for Postgres never sets POSTGRES_USER=keystone. This will fail in production. Fix: Establish admin credentials on Postgres service creation (write to service->credentials); pass via POSTGRES_USER/POSTGRES_PASSWORD in compose env.

10. Stateful update steps are placeholder strings that won't actually run

app/Actions/Services/CreateStatefulServiceUpdateOperation.php:38-42:

• 'docker compose down' — no -f path so it runs in the SSH user's home directory.
• 'docker volume ls' — listing isn't "preserving"; it's a no-op.
• 'docker compose up -d' — no -f path, no image digest, no env update.
• 'docker compose ps' — not a real health check.

Spec §11 specifies a real sequence: stop → preserve named volume → start new with updated digest → health check. Fix: Build steps from the driver against the service's actual compose path; verify the named volume exists before/after; replace healthcheck stub with docker inspect --format '{{.State.Health.Status}}' polling.

11. Stateful update doesn't write the updated digest into compose before restart

The operation sets available_image_digest on the service (line 55) but the compose file on disk is not re-rendered. docker compose up -d will pull from whatever digest is currently in the compose, not the new one. Fix: Insert a "Render compose with new digest" step before the start step.

12. Valkey driver doesn't emit role-based env vars

app/Drivers/Valkey/Valkey8Driver.php:42-48 only emits REDIS_HOST and REDIS_PORT. Spec §13 explicitly recommends CACHE_STORE=redis, SESSION_DRIVER=redis, QUEUE_CONNECTION=redis based on attachment role. Fix: Read the EnvironmentAttachment.role and add the appropriate Laravel env defaults (with the "Do not silently change queue behavior without confirmation" guard from §12).

13. Valkey has no logical-DB isolation

AttachManagedService.php:64-71 creates a logical_database slice but never assigns a Redis database index (REDIS_DB). All environments attached to the same Valkey service share DB 0. Fix: Assign REDIS_DB per slice; include in environmentExportsForSlice.

14. Caddy and Valkey slices have no SLICE_PROVISION operation

AttachManagedService::createSliceProvisionOperation (line 110) early-returns unless service->type === POSTGRES. Caddy routes and Valkey logical DBs are created in the DB but never reconciled to the running service. Fix: Emit slice operations for all service types whose driver supports slices.

15. Postgres driver doesn't export DB_* at the service level

Postgres18Driver::environmentExports() returns []. That's fine, but only slice exports work, so if a service has no slice but a Laravel app references DB_HOST, nothing wires it. Fix: Either guarantee a slice always exists for Postgres attachments, or emit DB_HOST at the service level via the attachment.

Deployment flow

16. Migration timing is hardcoded to pre_switch

DeployEnvironment::serviceDeployScripts (app/Jobs/Environments/DeployEnvironment.php:236-238) always emits the migration step before "Deploy replicas". Spec §18 lists migration_timing: pre_switch | post_switch on service config — never read. Fix: Check $service->config['migration_timing'] and either emit the migration step before replicas or after the gateway cutover.

17. Migration mode manual is ignored

migrationScript (line 292-301) only short-circuits when migration_mode=disabled. manual still auto-runs php artisan migrate --force during environment deploy. Spec §18 says manual mode should not run automatically. Fix: Treat manual the same as disabled for environment deploys; only the dedicated environment-migrations.store controller should run it.

18. Two parallel migration code paths

DeployEnvironment::migrationScript (line 292), LaravelRuntimeDriver::getOperationPlan, and EnvironmentMigrationController all emit migration scripts independently. They will drift. Fix: Centralize in one place (driver method or dedicated action) and call from all three.

19. "Update gateway routes" step is a no-op

DeployEnvironment.php:248-250:

'script' => 'test -f /home/keystone/gateway/Caddyfile',

This just checks file presence. The actual route update is in a separate slice_configure operation, so this step is dead code in the service-deploy chain. Fix: Either remove the step or have it actually trigger the route update for this service.

20. Pre-switch service steps from spec §17 step 6 are missing entirely

No "pre-switch" hooks are emitted by DeployEnvironment or any driver. Fix: Add a preSwitchSteps(): array driver capability and call it before the migration/replica steps.

21. Multi-server replica placement is not implemented

DeployEnvironment::ensureServiceReplicas (line 147-171) always assigns server_id = $service->server_id. There is no way to place replicas across multiple servers — yet the registry-required check at line 39-41 assumes multi-server deployments exist. Fix: Either ship single-server-only v1 and remove the multi-server gate, or wire Service.process_roles/placement policy to multiple servers in ensureServiceReplicas.

22. No explicit docker pull <ref>@<digest> on target servers for multi-server

replicaDeployScripts (line 261-290) does docker compose up -d only; for multi-server, target servers need to pull from the registry by digest. There is no pull step. Fix: Add docker pull step using registry_ref + digest before the up -d step on each target server.

23. Build strategy dedicated_builder and external_registry are not enforced

BuildApplicationArtifact::execute (app/Actions/Environments/BuildApplicationArtifact.php:30) picks any server via buildServer(). The push is only conditionally added when strategy === EXTERNAL_REGISTRY (line 89). There's no enforcement that dedicated_builder requires a builder service to exist, nor that external_registry skips local build entirely. Fix: Branch on strategy at the top of execute(); for external_registry, skip the build and resolve the digest from the registry (docker manifest inspect); for dedicated_builder, fail if no builder service is provisioned.

24. Scheduler placement is not enforced at runtime

Spec §8: single mode runs schedule:run on exactly one replica; every_replica runs on all. LaravelRuntimeDriver sets AUTORUN_LARAVEL_SCHEDULER=true based on the service's process_roles, but nothing applies the env per-replica based on scheduler_target_service_id/scheduler_mode. All replicas of the target service end up with the env var. Fix: When generating replica config, only emit AUTORUN_LARAVEL_SCHEDULER=true on the elected replica when scheduler_mode=single. The existing PlanEnvironmentDeployment::blockers (app/Actions/Environments/PlanEnvironmentDeployment.php:79-87) is a pre-flight check, not an enforcement.

25. Compose file is generated via shell heredoc instead of a real upload

composeUploadScript (line 303-319) inlines the compose body in a cat <<'KEYSTONE_COMPOSE' heredoc. Any quoting issue (binary, single-quote in env, large file) breaks. Spec §16 implies generated artifacts should be transferred via SSH/SCP. Fix: Use SCP (or ssh ... 'cat > path' with binary-safe encoding) instead of heredoc. Also drop separate generation of .env files — currently only the compose is uploaded; .env references in compose will 404 on disk.

26. .env files never written to disk

Spec §16 layout includes /home/keystone/services/<service-id>/.env. composeUploadScript writes only compose.yml. If the compose env_file: .env directive is rendered, the deploy will fail. Fix: Render and upload .env alongside compose.yml.

Models, schema, and encryption

27. Service.credentials migration column is plaintext but model casts as encrypted:array

Migration database/migrations/2025_03_27_121050_create_services_table.php:35 declares text('credentials'), while app/Models/Service.php:36 casts it as encrypted:array. The cast works (Laravel encrypts on write) — but a text column without explicit nullable()/encoding intent is confusing. Confirm cast actually encrypts (it does for encrypted:* casts) and document. Fix: Add ->nullable() and a comment in the migration noting the field is encrypted at the model layer.

28. EnvironmentVariable.value cast missing array-vs-string handling

Spec doesn't require it, but worth flagging: the cast is encrypted (scalar), which means complex values (JSON-encoded secrets) need explicit json_encode by callers. Currently AttachManagedService.php:97-104 always passes scalar values, so this is OK.

UI and onboarding

29. Onboarding (Spec §19) is not implemented

No onboarding controller, no routes, no Inertia pages. The spec calls for a guided flow: organisation → provider → source → deploy key → registry → server → app/env → attachments. Currently users must navigate disjoint pages manually. Fix: Add an OnboardingController with a state machine on Organisation (or session-based progress) plus an Inertia wizard.

30. Service detail/edit pages are missing

resources/js/pages/services/ only contains updates/Create.vue. There's no Index/Show/Edit. Spec §20 Phase 6 calls for "services under an environment with sensible defaults". Fix: Add service Show/Edit pages, including replica health, slices, and one-click update.

31. No "managed attachment" guided flow

resources/js/pages/environment-attachments/Create.vue exposes a raw service list. Spec §12 + §20 call for managed flows for Postgres / Valkey / Caddy with auto-defaulted slices. Fix: Build a guided picker per role (database / cache / queue / storage / gateway) that filters services to compatible types and previews the generated slice + env vars.

32. Deploy policies are visible / no defaults hiding

Spec §20 Phase 6: "Hide deploy policies by default." Currently they're set/exposed via Service form requests (StoreServiceRequest). No UI hiding. Fix: Don't expose deploy_policy in service create/edit UI; rely on driver-provided defaults from spec §2.

33. resources/js/pages/applications/Show.vue has a stale stub comment

Line 72: <!-- Add instance button would go here --> — references the removed Instance model. Fix: Remove the stale comment; add the actual "New environment" button.

34. resources/js/pages/servers/Index.vue contains a @todo pagination literal

Ship-ready code shouldn't have TODOs visible to the user. Fix: Either implement pagination or remove the comment.

35. No UI surfaces variable source / overridable

EnvironmentVariableController::store hardcodes source=USER. The UI in environment-variables/Create.vue provides no way to see managed vs. user vars, and the spec §13 requires the source/overridable badge. Fix: Read variables on the environment Show page grouped by source; for managed_attachment rows, show a "managed by Postgres slice X" badge and disable editing unless overridable.

36. No environment Show page; deploys/migrations etc. are triggered from applications/Show.vue per-environment row

This works but spec §20 Phase 6 wants environments to be the primary surface. Currently there's no environment detail page where services, replicas, slices, attachments, env vars, and operations are visible together. Fix: Add environments/Show.vue and route applications/Show.vue's environment row to it.

Tests — coverage gaps

37. EnvironmentDeploymentControllerTest only asserts dispatch

Short test, no state assertions after run. Fix: Replace Bus::fake() with running the job inline; assert that operations, steps, replica records, compose files, and env vars are all in expected state.

38. No test asserts deploy key cleanup after build

BuildApplicationArtifact.php:100-101 adds the cleanup trap. No test verifies the trap actually runs or that the key file is gone. Fix: In BuildApplicationArtifactTest, fake the remote runner and assert the build script contains trap cleanup EXIT AND that the operation_dir path resolves under /home/keystone/operations/.

39. No test asserts the named volume naming convention

Spec §10 names volumes keystone_service_<id>_postgres_data. No test in ComposeRendererTest checks volume name format. Fix: Snapshot-test or regex-assert the volume name pattern in ComposeRendererTest.

40. No test for parent-child operation chain executing end-to-end

DeployEnvironmentJobTest creates operations but never runs the resulting RunStep jobs through Queue::handleAll()-style flow. Fix: Add an integration test that fakes the SSH layer (return canned success) and lets the chain run, asserting each operation transitions to COMPLETED in the correct order.

41. No test for cancellation cascade on failure

None of the test files exercise RunStep::failed with sibling/child cancellation expectations (because that behavior doesn't exist yet — see gap #2). Fix: Add a test that fails a step mid-chain and asserts all later operations are CANCELLED.

42. No test for stateful update flow against a Postgres service

StatefulServiceUpdateTest likely asserts only operation/step rows. Need: rendered script asserts compose path is correct, named volume is preserved, new digest is written. Fix: Strengthen assertions to validate the full step contents.

43. No test for multi-server build/push/pull

BuildArtifactPlanningTest checks requiresRegistry=true but no test asserts a docker push and per-target docker pull actually occur. Fix: Add a job-level test with two-server topology and assert each target's deploy script includes a docker pull <ref>@<digest> before compose up.

44. No test that manual/disabled migration modes are honored

Fix: Parametrized test asserting migrationScript returns 'true' for disabled and manual modes, and the real command for auto.

45. No test for scheduler enforcement per replica

Fix: Test that for scheduler_mode=single, only one replica's rendered env has AUTORUN_LARAVEL_SCHEDULER=true; for every_replica, all do.

46. No test that managed attachment auto-creates slices for Valkey + Caddy

ManagedAttachmentTest likely tests Postgres only. Fix: Extend dataset to cover Valkey logical_database and Caddy route slices, with their env exports.

---

Suggested ordering

1. Fix the orchestration bugs (#1-#5) — without these the chain doesn't reliably reach completion.
2. Fix the Caddy cutover (#3, #4, #7, #19) — without these no environment can serve traffic.
3. Fix Postgres slice provision admin user (#9) and stateful update scripts (#10, #11).
4. Implement migration timing/mode (#16, #17, #18).
5. Implement scheduler enforcement (#24) and multi-server placement + pull (#21, #22, #23).
6. Then UI: onboarding (#29), environment Show page (#36), managed attachment UI (#31), variable source display (#35).
7. Strengthen tests last (#37-#46) once the orchestrator and drivers are stable.

Most of the schema and the high-level structure are correct — the gap is between the data model and the runtime behavior that's supposed to enforce/realize it.