Skip to content

Simplify V1 linked service manifests - #228

Merged
arjunkomath merged 4 commits into
mainfrom
feat/service-manifest-v1
Jul 29, 2026
Merged

Simplify V1 linked service manifests#228
arjunkomath merged 4 commits into
mainfrom
feat/service-manifest-v1

Conversation

@arjunkomath

@arjunkomath arjunkomath commented Jul 29, 2026

Copy link
Copy Markdown
Member

Summary

  • replace project/environment/service identity snapshots in techulus.yml with one immutable link at target.serviceId; everything else in the single manifest is desired service configuration
  • preserve authored configuration during tc link, reject relinking to a different service without an explicit unlink, and automatically select the environment when a project has exactly one
  • remove --force; tc apply always asks the backend to plan the complete desired state, shows every change, and applies only after explicit confirmation (--yes is required for noninteractive use)
  • make plan/apply concurrency-safe with canonical fingerprints, strict If-Match validation, bounded interactive replanning, and no silent materially changed retry under --yes
  • share canonical planning and validation between the plan and apply APIs, including deterministic source, hostname, port, placement, health-check, command, and resource diffs
  • serialize all managed service configuration writers with the same per-service advisory lock so a confirmed plan cannot race another control-plane write
  • use a concrete, DNS-safe hostname in manifests while preserving legacy name-derived hostname behavior and rename stability

This intentionally replaces the pre-alpha V1 manifest and API contracts without compatibility shims.

API scope

The service endpoints use /api/v1/services/{serviceId} rather than repeating project and environment IDs. This does not weaken authorization: API-key authorization is installation-wide today, not project-scoped, and every operation still loads and validates the target service before planning or applying.

Apply safety

  1. CLI sends the full desired configuration to POST /configuration/plan.
  2. Backend returns the authoritative target, complete structured diff, and current-state fingerprint.
  3. CLI renders the plan and requires y/yes, unless interactive confirmation was explicitly skipped with --yes.
  4. CLI applies with the quoted fingerprint in If-Match.
  5. If state changed, the backend returns CONFIGURATION_PLAN_STALE; interactive use replans and reconfirms, while --yes refuses a materially changed replacement plan.

A no-op plan does not prompt or send a PUT.

Validation

  • cd cli && go test ./...
  • cd cli && go build ./...
  • cd cli && test -z \"$(gofmt -l .)\"
  • cd web && ./node_modules/.bin/tsc --noEmit
  • cd web && mise exec -- pnpm test (47 files, 321 tests)
  • Biome check on all touched web files
  • git diff --check

Review

Oracle reviewed the complete plan/apply and linked-manifest design across multiple passes. Its findings around concurrent writers, transaction boundaries, stale-plan retries, null normalization, GitHub repository identity, hostname invariants, and deletion races were fixed and revalidated. The final review returned SHIP.

@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown

React Doctor found no issues. 🎉

⚠️ Warning: .github/workflows/react-doctor.yml is configured incorrectly. See below to fix.

React Doctor compares against main to report only the issues this pull request introduces. This run couldn't complete that comparison (usually a shallow CI checkout with no merge base), so it listed every issue in the changed files, including ones that already existed on main.

Add fetch-depth: 0 to the actions/checkout step in .github/workflows/react-doctor.yml so the checkout includes the history React Doctor needs:

 jobs:
   react-doctor:
     steps:
       - uses: actions/checkout@v5
+        with:
+          fetch-depth: 0

       - uses: millionco/react-doctor@v2

To silence this warning, set silence-missing-baseline-warning: true on the React Doctor action.

Reviewed by React Doctor for commit cdcfb78.

@mintlify

mintlify Bot commented Jul 29, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
techulus-cloud 🟢 Ready View Preview Jul 29, 2026, 9:29 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@techulus-agent techulus-agent left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Overview

Collapses the manifest's project/environment/service identity triple down to target.serviceId, flattens service routes to /api/v1/services/{serviceId}, and turns PATCH /configuration into a strict full-replacement PUT. Responses now carry an authoritative target so the CLI stops printing identity it cached locally. The direction is right — the old manifest duplicated server-owned identity in three places and could silently drift — and given the stated pre-alpha no-shims policy, a clean break is the correct call.

Two things below I'd want resolved before merge; the rest are smaller.


1. tc init + tc link will silently rename the remote service on the next tc apply

This is the interaction of two individually-reasonable changes: name is now part of the strict PUT body, and finishLink preserves the local service block when a manifest already exists.

Concretely:

tc init                    # writes service.name: <folder-name>
tc link --service abc123   # existing != nil, not Linked() -> keeps local service block, sets target only
tc apply                   # PUTs {"name": "<folder-name>", ...} -> renames the remote service

The new test encodes exactly this: the remote service is named remote, and after link --service s the assertion is reflect.DeepEqual(linked.Manifest.Service, before.Manifest.Service) — i.e. the manifest keeps the init-generated name. Then printLinked reports app/prod/remote, the remote name, so the output actively conceals the divergence the user is now carrying.

The same applies to --force rebinds, and there it's broader than the name: after tc link --service other --force, the manifest still holds the previous service's source, ports, placement, and resources, and because PUT is now full-replacement, the next tc apply overwrites other's entire managed configuration. changed() reports it in the response, but only after the write has happened.

I don't think preserving the desired configuration is wrong — it's the point of the feature — but it needs a guard. Options, roughly in order of preference:

  • Have tc link print the divergence it's creating (service.name: my-folder vs remote remote) and require confirmation, or
  • Adopt the remote name into the manifest on link even when preserving the rest, since name is server identity in a way that ports/resources aren't, or
  • Make tc apply show the pending change set and confirm before writing.

Related: --force's help text still reads "Replace an existing techulus.yml", but the flag now means "rebind to a different service" — the old "file already exists, use --force" guard is gone entirely.

2. Default internal hostname changes for every existing null-hostname service

getDefaultServiceHostname(service.name)getDefaultServiceHostname(service.id) at both call sites (lib/public-api.ts:462, lib/service-revision-spec.ts:210). The function is a slugifier, so feeding it a UUID returns the UUID unchanged.

The stated goal — a default hostname that survives a rename — is achieved, but there are two consequences the summary doesn't mention:

  • Existing services drift. Any service with a null hostname has an active revision spec whose hostname is name-derived (hello-service). The new comparable computes the UUID instead, so those services will now report hasPendingChanges: true, and the next deploy rewrites their internal DNS name. Any sibling service resolving hello-service.internal breaks at that moment. Pre-alpha policy covers the API contract; this is a data-plane change to already-running installs, which is a different kind of break. Worth confirming it's intended and, if so, calling it out in the PR description.
  • The default becomes unusable as a DNS name. 0400075c-69aa-46c2-bccc-fc172b8c6b28.internal isn't something anyone types. Combined with #226 removing the .internal display from service cards, a user with a default hostname now has no practical way to discover or use it. A rename-stable and human-usable option would be to slugify the name once at creation and persist it, rather than deriving it at read time.

Minor: the parameter is still named name in getDefaultServiceHostname(name: string), so passing an ID typechecks silently. Rename the parameter, or introduce a distinct function, so the next reader doesn't "fix" it back.


Smaller findings

  • Dead code in environmentsCommand. The diff leaves:
    if id == "" { return errors.New("missing --project") }
    if id == "" { return errors.New("missing --project (or link this directory)") }
    The second block is unreachable, and its message still promises the manifest fallback that was just removed. Delete it.
  • resolveServiceTarget fabricates Service{Name: service} — the service ID stored in the Name field. It's currently harmless because apply has no --service flag (addServiceTargetFlags is only wired to status/logs/rollout) and status/logs now render result.Target. But with name in the PUT body, any future command that both accepts --service and applies configuration would rename the service to its own ID. Leaving Name empty would be safer than filling it with something false.
  • serviceBase returns /api/v1/services/ when Target is nil rather than erroring. Unreachable today thanks to the Linked() check, but a trailing-slash request is a bad failure mode; returning an error would be clearer.
  • Vestigial optionality after the schema tightening. input.source?.type === "image" and input.hostname !== undefined in replaceConfiguration are now always-true/always-defined, since the schema makes both required. Harmless but misleading about the contract.
  • explicitIDs in linkCommand is now an int that only ever holds 0 or 1. A bool would read better.
  • tc environments / tc services losing their manifest fallback is an unavoidable consequence of dropping the IDs, and the errors are accurate. Fine.

Verified, not a problem

  • Authorization. findNestedService enforced project+environment containment; findServiceContext looks a service up by ID alone. This is not a privilege escalation — the API's own docs state roles are global with no project-level permissions, so containment was validation rather than an authz boundary, and every role that can now read a service by ID could already read it via the nested path. Worth stating in the PR description since the diff looks alarming out of context.
  • findServiceContext still filters isNull(services.deletedAt), and the redundant environments.projectId = projects.id join condition is a harmless consistency guard.
  • Resource null-normalization in replaceConfiguration (persisted {null, null}null before comparison) is correct and avoids a spurious change entry.

Tests

Good coverage on the parts that matter: the direct-link/rebind/force matrix, the unmanaged-service path asserting the manifest is left byte-identical on failure, the strict-schema rejections, and rename-stability of the hostname. The one gap is the scenario in #1 — there's no test asserting what tc apply sends after tc init + tc link, which is precisely where the surprise lives.

Docs

docs/api/public-api.mdx is updated thoroughly and matches the implementation, including the rootDir required-nullable change and the single-environment auto-select behaviour. Nice.

Verdict

Sound refactor. I'd resolve the link/rename interaction and get a decision on the hostname migration before merging; the rest are cleanups.

@techulus-agent techulus-agent left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review (4f1aaa1 — plan and confirm manifest apply)

Both earlier findings are addressed, and in better ways than I suggested.

The silent-rename problem is gone at the root: tc apply now POSTs /configuration/plan, renders every change as field: from -> to, and prompts. A rename surfaces as name: old-web -> web before anything is written. On top of that, --force rebinding is removed entirely (relinking requires deleting target.serviceId by hand), and the write is guarded by If-Match on a currentVersion fingerprint with a 409 CONFIGURATION_PLAN_STALE replan loop. The detail I like most is that --yes refuses to auto-apply a replanned diff — automation can't silently write something the operator never saw. That's a stronger guarantee than the confirmation prompt I asked for.

The hostname problem is solved by making hostnames concrete rather than derived: getDefaultServiceHostname goes back to name-derived (so existing services keep hello-service, no data-plane drift), the ID is only a fallback for empty/non-ASCII names, and there's now 63-char truncation with an ID suffix. Meanwhile hostname became required and non-null through the manifest, the PUT schema, and tc init, and replaceConfiguration materializes it on first apply via if (!persisted.hostname?.trim() || hostnameChanged). The derived default stops being a moving target instead of being made stable-but-unusable. Good call.

Also fixed: resolveServiceTarget no longer fabricates Name: <serviceId>, explicitIDs is a bool, the vestigial input.source?. optionality is cleaned up, and the flat-route authorization model is now stated explicitly in the docs.


New findings

1. validateDockerImageInternal now runs on every plan and every apply. In replaceConfigurationInternal the guard dropped from input.source.image !== service.image to just input.source.type === "image". Since tc apply calls plan then PUT, an image service makes two external registry round-trips per invocation — including when the plan is a no-op and nothing is written. Restoring the !== persisted.image condition needs the persisted row, so it'd have to move inside the transaction, but it's worth doing; this is on the hot path for every apply.

2. Human-mode tc apply prints nothing on success, and the PUT response is discarded.

var result applyResponse
err = client.RequestJSONWithHeaders(..., http.MethodPut, base, ..., &result)
...
if a.isMachineOutput() { return a.writeData(plan, "Applied") }
return nil

result is decoded and never read. In human mode you answer y and get silence — no confirmation that anything happened. In machine mode the envelope is labelled "Applied" but carries the pre-apply plan, so any divergence between what the server planned and what it actually wrote is invisible to both audiences. Print result (or at least an "Applied N changes" line) rather than echoing the plan.

3. serviceBase panics on a missing target. It's genuinely unreachable behind the Linked() check, but a panic in a CLI means the user gets a goroutine dump instead of a message. An error return costs nothing here.

4. Port diffs will render badly. configurationChanges recurses into plain objects but compares arrays atomically, so a port change is one entry whose from/to are whole arrays. printApplyResult formats those with %v, so the terminal shows ports: [map[containerPort:8080 domain:<nil> public:false]] -> [...]. The plan output is the centrepiece of this commit — ports deserve either element-wise diffing or a dedicated formatter.

5. output.Error uses a direct type assertion (err.(errorWithPlan)) rather than errors.As. It works today because applyPlanError reaches the writer unwrapped, but any future fmt.Errorf("%w") silently drops the plan from the JSON envelope.

6. Carried over, still unfixed: the unreachable second block in environmentsCommand (cli/internal/cli/app.go:811-816) — two consecutive if id == "" returns, and the dead one's message still advertises the manifest fallback that no longer exists.

7. Minor: hashtext() returns int4, so distinct service IDs can collide and serialize against each other. Correctness is unaffected, it's just occasional false contention — not worth changing, just worth knowing.

Verified

  • Advisory lock placement is right. pg_advisory_xact_lock is the first statement in replaceConfigurationInternal's transaction, before the persisted read, so the If-Match staleness check is genuinely serialized against the UI mutation paths that take the same lock. This was my main worry when I saw the ETag and the locks arrive as separate mechanisms.
  • Hostname uniqueness is backstopped by the DB (services.hostname is .unique()), so the in-transaction duplicate check is a friendlier error rather than the sole guard — the cross-service race the advisory lock can't cover is handled.
  • hostnameSchema (max 63, ^[a-z0-9]+(?:-[a-z0-9]+)*$) matches the CLI's hostnamePattern and length cap exactly, so client and server reject the same values.
  • updateServiceName materializes from current.name, not validatedName — i.e. the pre-rename derived hostname is frozen, so renaming doesn't move DNS. Subtle and correct; worth a comment so it doesn't get "fixed" later.
  • createService routing through getDefaultServiceHostname fixes the unbounded ${slug}-${name}-${env} that could exceed 63 chars.

Tests

The stale-plan matrix is thorough — interactive replan-and-reconfirm, --yes refusing to write a replanned diff, the bounded-retry ceiling, and the machine-mode error envelope carrying the replacement plan. TestApplyPlansAndRequiresConfirmation covers no-op, decline, and non-interactive. The canonicalization tests (ordering stability, GitHub casing, omitted-domain normalization) are the right shape for a fingerprint that gates writes.

One gap matching finding #2: no test asserts what a human sees after a successful apply, which is why the empty success path went unnoticed.

Verdict

The two blockers from my last pass are resolved. Everything above is small — #1 and #2 are the ones I'd fix before merge.

Amp-Thread-ID: https://ampcode.com/threads/T-019fac48-72fe-7706-9ce1-45eb90eadbef
Co-authored-by: Arjun Komath <arjunkomath@gmail.com>
@arjunkomath
arjunkomath added this pull request to the merge queue Jul 29, 2026
Merged via the queue into main with commit eb74259 Jul 29, 2026
15 checks passed
@arjunkomath
arjunkomath deleted the feat/service-manifest-v1 branch July 29, 2026 11:30
@mintlify

mintlify Bot commented Jul 29, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
techulus-cloud 🟡 Building Jul 29, 2026, 9:28 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants