From d63e2a6d7924551b3eaf470db3d920591eaf0df1 Mon Sep 17 00:00:00 2001 From: untra Date: Sun, 13 Sep 2026 19:09:23 -0600 Subject: [PATCH 1/4] chart fixes further --- .github/workflows/build.yaml | 163 ++++++++++++---- Dockerfile | 5 +- bindings/AgentState.ts | 9 +- bindings/RestApiConfig.ts | 10 +- bindings/SetupExecutionTarget.ts | 2 +- bindings/ShutdownRecovery.ts | 3 + charts/operator/README.md | 57 ++++++ charts/operator/templates/NOTES.txt | 23 +++ charts/operator/templates/_helpers.tpl | 29 +++ charts/operator/templates/statefulset.yaml | 17 +- charts/operator/values.schema.json | 23 +++ charts/operator/values.yaml | 7 + docs/getting-started/platforms/kubernetes.md | 54 +++++- docs/schemas/openapi.json | 19 ++ src/agents/launcher/mod.rs | 29 +++ src/config.rs | 20 ++ src/rest/dto/setup.rs | 7 +- src/rest/error.rs | 3 + src/rest/mod.rs | 178 +++++++++++++++++- src/rest/routes/launch.rs | 52 +++-- src/rest/routes/probes.rs | 27 +++ src/rest/routes/setup.rs | 43 ++++- src/rest/state.rs | 151 +++++++++++++++ src/state.rs | 15 +- src/ui/in_progress_panel.rs | 1 + src/ui/session_preview.rs | 1 + tests/distribution_bundling.rs | 9 +- .../onboarding/OnboardingPage.module.css | 5 +- ui/src/routes/onboarding/OnboardingPage.tsx | 24 ++- ui/src/routes/onboarding/steps.tsx | 77 +++++++- ui/src/routes/onboarding/types.ts | 1 + 31 files changed, 990 insertions(+), 74 deletions(-) create mode 100644 bindings/ShutdownRecovery.ts create mode 100644 charts/operator/README.md create mode 100644 charts/operator/templates/NOTES.txt diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 0e6eeb7e..ae994ce4 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -153,6 +153,25 @@ jobs: --set networkPolicy.enabled=true \ --set bootstrap.existingSecret=operator-bootstrap \ > operator-chart.yaml + helm template operator charts/operator \ + --set-json 'extraVolumes=[{"name":"custom-ca","configMap":{"name":"custom-ca"}}]' \ + --set-json 'extraVolumeMounts=[{"name":"custom-ca","mountPath":"/etc/custom-ca","readOnly":true}]' \ + --set-json 'lifecycle={"preStop":{"exec":{"command":["/bin/sh","-c","true"]}}}' \ + > operator-chart-extensions.yaml + if helm template operator charts/operator --set terminationGracePeriodSeconds=75; then + echo "::error::invalid shutdown budget rendered successfully" + exit 1 + fi + if helm template operator charts/operator \ + --set-json 'extraVolumes=[{"name":"workspace","emptyDir":{}}]'; then + echo "::error::chart-owned volume collision rendered successfully" + exit 1 + fi + if helm template operator charts/operator \ + --set-json 'extraVolumeMounts=[{"name":"one","mountPath":"/shared"},{"name":"two","mountPath":"/shared"}]'; then + echo "::error::duplicate mount path rendered successfully" + exit 1 + fi - name: Scan rendered Helm chart uses: aquasecurity/trivy-action@v0.36.0 @@ -672,67 +691,135 @@ jobs: docker: needs: [build, build-opr8r, release] - runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + include: + - arch: amd64 + runner: ubuntu-latest + operator_artifact: operator-linux-x86_64 + opr8r_artifact: opr8r-linux-x86_64 + - arch: arm64 + runner: ubuntu-24.04-arm + operator_artifact: operator-linux-arm64 + opr8r_artifact: opr8r-linux-arm64 + runs-on: ${{ matrix.runner }} steps: - uses: actions/checkout@v7 + with: + ref: ${{ needs.release.outputs.commit }} - - name: Download linux operator binaries + - name: Download operator binary uses: actions/download-artifact@v8 with: - pattern: operator-linux-* + name: ${{ matrix.operator_artifact }} path: bins - merge-multiple: true - # The image ships both halves of the server-client pair, so agent - # sessions inside the container can report step completion via opr8r. - - name: Download linux opr8r binaries + - name: Download opr8r binary uses: actions/download-artifact@v8 with: - pattern: opr8r-linux-* + name: ${{ matrix.opr8r_artifact }} path: bins - merge-multiple: true - # buildx exposes TARGETARCH as amd64/arm64; map the x86_64 artifact name. - name: Stage binaries for build context run: | - cp bins/operator-linux-x86_64 ./operator-linux-amd64 - cp bins/operator-linux-arm64 ./operator-linux-arm64 - cp bins/opr8r-linux-x86_64 ./opr8r-linux-amd64 - cp bins/opr8r-linux-arm64 ./opr8r-linux-arm64 - chmod +x operator-linux-amd64 operator-linux-arm64 \ - opr8r-linux-amd64 opr8r-linux-arm64 + cp "bins/${{ matrix.operator_artifact }}" "operator-linux-${{ matrix.arch }}" + cp "bins/${{ matrix.opr8r_artifact }}" "opr8r-linux-${{ matrix.arch }}" + chmod +x "operator-linux-${{ matrix.arch }}" "opr8r-linux-${{ matrix.arch }}" - - uses: docker/setup-qemu-action@v4 - uses: docker/setup-buildx-action@v4 - - name: Log in to Docker Hub - uses: docker/login-action@v4 - with: - username: ${{ secrets.DOCKERHUB_USERNAME }} - password: ${{ secrets.DOCKERHUB_TOKEN }} - - - name: Build and push multi-arch image + - name: Build image for scanning uses: docker/build-push-action@v7 with: context: . - platforms: linux/amd64,linux/arm64 - push: true - tags: | - untra/operator:${{ needs.release.outputs.version }} - untra/operator:latest + platforms: linux/${{ matrix.arch }} + load: true + tags: operator-ci:${{ needs.release.outputs.version }}-${{ matrix.arch }} - name: Scan container image uses: aquasecurity/trivy-action@v0.36.0 with: - image-ref: untra/operator:${{ needs.release.outputs.version }} + image-ref: operator-ci:${{ needs.release.outputs.version }}-${{ matrix.arch }} scanners: vuln severity: HIGH,CRITICAL ignore-unfixed: true + format: sarif + output: trivy-${{ matrix.arch }}.sarif exit-code: '1' - chart: + - name: Upload scan report + if: always() + uses: actions/upload-artifact@v7 + with: + name: trivy-${{ matrix.arch }} + path: trivy-${{ matrix.arch }}.sarif + if-no-files-found: ignore + retention-days: 14 + + - name: Export scanned image + run: docker save "operator-ci:${{ needs.release.outputs.version }}-${{ matrix.arch }}" -o "operator-${{ matrix.arch }}.tar" + + - name: Upload scanned image + uses: actions/upload-artifact@v7 + with: + name: operator-image-${{ matrix.arch }} + path: operator-${{ matrix.arch }}.tar + compression-level: 0 + retention-days: 1 + + - name: Report scan result + if: always() + run: echo "### linux/${{ matrix.arch }} blocking Trivy scan โ€” ${{ job.status }}" >> "$GITHUB_STEP_SUMMARY" + + docker-publish: needs: [release, docker] runs-on: ubuntu-latest + outputs: + digest: ${{ steps.manifest.outputs.digest }} + steps: + - uses: actions/download-artifact@v8 + with: + pattern: operator-image-* + path: images + merge-multiple: true + + - name: Log in to Docker Hub + uses: docker/login-action@v4 + with: + username: ${{ secrets.DOCKERHUB_USERNAME }} + password: ${{ secrets.DOCKERHUB_TOKEN }} + + - name: Push scanned platform images + id: platforms + run: | + for arch in amd64 arm64; do + docker load -i "images/operator-${arch}.tar" + source="operator-ci:${{ needs.release.outputs.version }}-${arch}" + candidate="untra/operator:${{ needs.release.outputs.version }}-${arch}-${{ github.run_id }}" + docker tag "$source" "$candidate" + docker push "$candidate" + digest=$(docker buildx imagetools inspect "$candidate" --format '{{json .Manifest.Digest}}' | tr -d '"') + echo "${arch}=untra/operator@${digest}" >> "$GITHUB_OUTPUT" + done + + - name: Publish multi-platform manifests + id: manifest + run: | + version="${{ needs.release.outputs.version }}" + amd64="${{ steps.platforms.outputs.amd64 }}" + arm64="${{ steps.platforms.outputs.arm64 }}" + docker buildx imagetools create -t "untra/operator:${version}" -t untra/operator:latest "$amd64" "$arm64" + digest=$(docker buildx imagetools inspect "untra/operator:${version}" --format '{{json .Manifest.Digest}}' | tr -d '"') + echo "digest=$digest" >> "$GITHUB_OUTPUT" + docker buildx imagetools inspect "untra/operator:${version}" + docker buildx imagetools inspect "untra/operator:${version}" --raw \ + | jq -e '[.manifests[].platform | select(.os == "linux") | .architecture] | sort == ["amd64", "arm64"]' + echo "### Published untra/operator:${version} at ${digest}" >> "$GITHUB_STEP_SUMMARY" + + chart: + needs: [release, docker-publish] + runs-on: ubuntu-latest permissions: contents: read packages: write @@ -752,8 +839,20 @@ jobs: helm package charts/operator --destination dist helm push "dist/operator-${{ needs.release.outputs.version }}.tgz" oci://ghcr.io/untra/charts + - name: Verify public chart + run: | + mkdir -p /tmp/helm-public + export HELM_REGISTRY_CONFIG=/tmp/helm-public/config.json + version="${{ needs.release.outputs.version }}" + helm pull oci://ghcr.io/untra/charts/operator --version "$version" --destination /tmp/helm-public + chart_version=$(helm show chart "/tmp/helm-public/operator-${version}.tgz" | awk '/^version:/ {print $2}') + image=$(helm template verify "/tmp/helm-public/operator-${version}.tgz" | awk '/image: "untra\/operator:/ {print $2; exit}' | tr -d '"') + test "$chart_version" = "$version" + test "$image" = "untra/operator:$version" + echo "### Published and anonymously verified operator chart ${version}" >> "$GITHUB_STEP_SUMMARY" + deploy-docs: - needs: release + needs: [release, chart] runs-on: ubuntu-latest steps: - name: Trigger docs workflow diff --git a/Dockerfile b/Dockerfile index d1becffb..f7834ebd 100644 --- a/Dockerfile +++ b/Dockerfile @@ -10,11 +10,10 @@ LABEL org.opencontainers.image.title="Operator" \ # Populated automatically by buildx per target platform (amd64 / arm64). ARG TARGETARCH -# Substrate Operator needs to launch agents: git (VCS ops), tmux (session -# wrapper), ca-certificates (TLS to LLM/kanban APIs), openssh-client (every -# ssh and coder target launch, and git over SSH remotes), curl (in-pod reachability checks). +# Substrate Operator needs to launch agents: git (VCS ops), tmux (session wrapper), ca-certificates (TLS to LLM/kanban APIs), openssh-client, curl, etc. # The LLM CLI (claude / codex / gemini) and its auth are supplied by the user via a derived image or env vars RUN apt-get update \ + && apt-get upgrade -y --no-install-recommends \ && apt-get install -y --no-install-recommends ca-certificates curl git openssh-client tmux \ && rm -rf /var/lib/apt/lists/* diff --git a/bindings/AgentState.ts b/bindings/AgentState.ts index a6c1801b..b021d0d8 100644 --- a/bindings/AgentState.ts +++ b/bindings/AgentState.ts @@ -1,5 +1,6 @@ // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. import type { GitExecutionConfig } from "./GitExecutionConfig"; +import type { ShutdownRecovery } from "./ShutdownRecovery"; import type { StepLaunchContext } from "./StepLaunchContext"; export type AgentState = { @@ -98,6 +99,10 @@ remote_host: string | null, */ step_launch_context: StepLaunchContext | null, /** - * Name of the resolved execution target this agent launched on + * Name of the resolved execution target this agent launched on. */ -target_name: string | null, }; +target_name: string | null, +/** + * Shutdown recovery strategy. + */ +shutdown_recovery?: ShutdownRecovery | null, }; diff --git a/bindings/RestApiConfig.ts b/bindings/RestApiConfig.ts index 0f27f64b..c06dfad4 100644 --- a/bindings/RestApiConfig.ts +++ b/bindings/RestApiConfig.ts @@ -23,4 +23,12 @@ cors_origins: Array, /** * Externally reachable base URL (e.g. `https://operator.example.com`). Defaults to request host. */ -public_url: string | null, }; +public_url: string | null, +/** + * Maximum time to wait for active agents before shutdown cleanup begins. + */ +shutdown_drain_seconds: bigint, +/** + * Maximum time reserved for final callbacks and persistent cleanup. + */ +shutdown_cleanup_seconds: bigint, }; diff --git a/bindings/SetupExecutionTarget.ts b/bindings/SetupExecutionTarget.ts index 24178d67..61c6e5ad 100644 --- a/bindings/SetupExecutionTarget.ts +++ b/bindings/SetupExecutionTarget.ts @@ -1,3 +1,3 @@ // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. -export type SetupExecutionTarget = { "kind": "local" } | { "kind": "coder", name: string, template: string, }; +export type SetupExecutionTarget = { "kind": "local" } | { "kind": "coder", name: string, template: string, parameters: { [key in string]: string }, }; diff --git a/bindings/ShutdownRecovery.ts b/bindings/ShutdownRecovery.ts new file mode 100644 index 00000000..53f9f39d --- /dev/null +++ b/bindings/ShutdownRecovery.ts @@ -0,0 +1,3 @@ +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. + +export type ShutdownRecovery = "interrupted_local" | "remote_awaiting_reconciliation"; diff --git a/charts/operator/README.md b/charts/operator/README.md new file mode 100644 index 00000000..c7ad064b --- /dev/null +++ b/charts/operator/README.md @@ -0,0 +1,57 @@ +# Operator Helm chart + +This chart installs Operator as a single-replica StatefulSet with a persistent workspace volume and authenticated HTTP API. + +```bash +kubectl create namespace operator +kubectl -n operator create secret generic operator-bootstrap \ + --from-literal=password="$(openssl rand -base64 24)" +helm install operator oci://ghcr.io/untra/charts/operator \ + --namespace operator \ + --set bootstrap.existingSecret=operator-bootstrap +kubectl -n operator port-forward service/operator 7008:7008 +``` + +Open `http://127.0.0.1:7008/setup`, set the admin password, and complete the workspace wizard. Delete the temporary bootstrap Secret afterward. The wizard writes `/op/.tickets/operator/config.toml`; the chart does not project or override that file. Back up the workspace PVC. + +## Common values + +| Value | Default | Purpose | +| --- | --- | --- | +| `image.repository` | `untra/operator` | Container image repository | +| `image.tag` | chart `appVersion` | Exact image version | +| `publicUrl` | empty | External URL used in generated links | +| `persistence.size` | `20Gi` | Workspace PVC size | +| `extraEnv` / `extraEnvFrom` | `[]` | Additional environment configuration | +| `extraVolumes` / `extraVolumeMounts` | `[]` | Additional Secrets, ConfigMaps, and volumes | +| `terminationGracePeriodSeconds` | `90` | Kubernetes termination budget | +| `shutdownDrainSeconds` | `60` | Time allowed for agents to finish | +| `shutdownCleanupSeconds` | `15` | Time reserved for cleanup and persistence | + +`terminationGracePeriodSeconds` must be greater than the sum of the two shutdown intervals. A custom `lifecycle` hook consumes the same Kubernetes grace period. + +## Custom trust and SSH material + +Mount a CA bundle that contains both public and internal roots, then configure each client that needs it: + +```yaml +extraEnv: + - name: SSL_CERT_FILE + value: /etc/operator-ca/ca-bundle.crt + - name: GIT_SSL_CAINFO + value: /etc/operator-ca/ca-bundle.crt +extraVolumes: + - name: operator-ca + configMap: + name: operator-ca +extraVolumeMounts: + - name: operator-ca + mountPath: /etc/operator-ca + readOnly: true +``` + +Verify `operator`, `curl`, and `git` independently because they may use different TLS implementations. Merely mounting a certificate does not add it to the system trust store. + +For Git over SSH, mount a Secret containing a private key and pinned `known_hosts`, readable by UID/GID 10001, then set `GIT_SSH_COMMAND` to use both files with `StrictHostKeyChecking=yes`. Do not disable host-key verification. + +The chart deliberately does not create RBAC or mount a service-account token. See the Kubernetes guide at https://operator.untra.io/getting-started/platforms/kubernetes/ for ingress, NetworkPolicy, Coder integration, backup, and restore details. diff --git a/charts/operator/templates/NOTES.txt b/charts/operator/templates/NOTES.txt new file mode 100644 index 00000000..c6bf7db1 --- /dev/null +++ b/charts/operator/templates/NOTES.txt @@ -0,0 +1,23 @@ +Operator is installed in namespace {{ .Release.Namespace }}. + +Wait for the pod: + kubectl -n {{ .Release.Namespace }} rollout status statefulset/{{ include "operator.fullname" . }} + +{{- if .Values.ingress.enabled }} +Open {{ printf "%s://%s%s/setup" (ternary "https" "http" (ne .Values.ingress.tls.secretName "")) .Values.ingress.host (trimSuffix "/" .Values.ingress.path) }} to finish bootstrap and the workspace wizard. +{{- else }} +Forward the service locally: + kubectl -n {{ .Release.Namespace }} port-forward service/{{ include "operator.fullname" . }} 7008:{{ .Values.service.port }} + +Then open http://127.0.0.1:7008/setup to finish bootstrap and the workspace wizard. +{{- end }} +{{- with .Values.publicUrl }} + +Operator will use {{ trimSuffix "/" . }}/setup in generated public links. publicUrl does not create an Ingress. +{{- end }} + +The wizard stores configuration and application state on the workspace PVC. Back up that volume. +{{- if not .Values.bootstrap.existingSecret }} + +No bootstrap Secret is configured. If this is a fresh installation, see the chart README before opening /setup. +{{- end }} diff --git a/charts/operator/templates/_helpers.tpl b/charts/operator/templates/_helpers.tpl index 64fe56a0..92551951 100644 --- a/charts/operator/templates/_helpers.tpl +++ b/charts/operator/templates/_helpers.tpl @@ -30,3 +30,32 @@ app.kubernetes.io/instance: {{ .Release.Name }} {{- define "operator.serviceAccountName" -}} {{- default (include "operator.fullname" .) .Values.serviceAccount.name }} {{- end }} + +{{- define "operator.validatePodExtensions" -}} +{{- $ownedVolumes := dict "workspace" true "home" true "tmp" true "bootstrap" true -}} +{{- $volumeNames := dict -}} +{{- range .Values.extraVolumes -}} + {{- if hasKey $ownedVolumes .name -}} + {{- fail (printf "extraVolumes name %q conflicts with a chart-owned volume" .name) -}} + {{- end -}} + {{- if hasKey $volumeNames .name -}} + {{- fail (printf "extraVolumes contains duplicate name %q" .name) -}} + {{- end -}} + {{- $_ := set $volumeNames .name true -}} +{{- end -}} +{{- $ownedMountPaths := dict "/op" true "/home/operator" true "/tmp" true "/run/secrets/operator-bootstrap" true -}} +{{- $mountPaths := dict -}} +{{- range .Values.extraVolumeMounts -}} + {{- if hasKey $ownedMountPaths .mountPath -}} + {{- fail (printf "extraVolumeMounts mountPath %q conflicts with a chart-owned mount" .mountPath) -}} + {{- end -}} + {{- if hasKey $mountPaths .mountPath -}} + {{- fail (printf "extraVolumeMounts contains duplicate mountPath %q" .mountPath) -}} + {{- end -}} + {{- $_ := set $mountPaths .mountPath true -}} +{{- end -}} +{{- $shutdownBudget := add .Values.shutdownDrainSeconds .Values.shutdownCleanupSeconds -}} +{{- if le (int .Values.terminationGracePeriodSeconds) (int $shutdownBudget) -}} + {{- fail "terminationGracePeriodSeconds must be greater than shutdownDrainSeconds + shutdownCleanupSeconds" -}} +{{- end -}} +{{- end -}} diff --git a/charts/operator/templates/statefulset.yaml b/charts/operator/templates/statefulset.yaml index 067d24c9..8022aa30 100644 --- a/charts/operator/templates/statefulset.yaml +++ b/charts/operator/templates/statefulset.yaml @@ -1,4 +1,5 @@ apiVersion: apps/v1 +{{- include "operator.validatePodExtensions" . }} kind: StatefulSet metadata: name: {{ include "operator.fullname" . }} @@ -32,7 +33,7 @@ spec: automountServiceAccountToken: false securityContext: {{- toYaml .Values.podSecurityContext | nindent 8 }} - terminationGracePeriodSeconds: 30 + terminationGracePeriodSeconds: {{ .Values.terminationGracePeriodSeconds }} {{- with .Values.imagePullSecrets }} imagePullSecrets: {{- toYaml . | nindent 8 }} @@ -43,6 +44,10 @@ spec: imagePullPolicy: {{ .Values.image.pullPolicy }} args: - api + {{- with .Values.lifecycle }} + lifecycle: + {{- toYaml . | nindent 12 }} + {{- end }} securityContext: {{- toYaml .Values.containerSecurityContext | nindent 12 }} ports: @@ -60,6 +65,10 @@ spec: value: 0.0.0.0 - name: OPERATOR_REST_API__PORT value: {{ .Values.service.port | quote }} + - name: OPERATOR_REST_API__SHUTDOWN_DRAIN_SECONDS + value: {{ .Values.shutdownDrainSeconds | quote }} + - name: OPERATOR_REST_API__SHUTDOWN_CLEANUP_SECONDS + value: {{ .Values.shutdownCleanupSeconds | quote }} {{- with .Values.publicUrl }} - name: OPERATOR_REST_API__PUBLIC_URL value: {{ . | quote }} @@ -101,6 +110,9 @@ spec: mountPath: /run/secrets/operator-bootstrap readOnly: true {{- end }} + {{- with .Values.extraVolumeMounts }} + {{- toYaml . | nindent 12 }} + {{- end }} volumes: - name: home emptyDir: {} @@ -115,6 +127,9 @@ spec: - key: {{ $.Values.bootstrap.passwordKey | quote }} path: password {{- end }} + {{- with .Values.extraVolumes }} + {{- toYaml . | nindent 8 }} + {{- end }} volumeClaimTemplates: - metadata: name: workspace diff --git a/charts/operator/values.schema.json b/charts/operator/values.schema.json index 45fa5d21..4c45fa71 100644 --- a/charts/operator/values.schema.json +++ b/charts/operator/values.schema.json @@ -68,6 +68,29 @@ "resources": { "type": "object" }, "extraEnv": { "type": "array", "items": { "type": "object" } }, "extraEnvFrom": { "type": "array", "items": { "type": "object" } }, + "extraVolumes": { + "type": "array", + "items": { + "type": "object", + "required": ["name"], + "properties": { "name": { "type": "string", "minLength": 1 } } + } + }, + "extraVolumeMounts": { + "type": "array", + "items": { + "type": "object", + "required": ["name", "mountPath"], + "properties": { + "name": { "type": "string", "minLength": 1 }, + "mountPath": { "type": "string", "pattern": "^/" } + } + } + }, + "lifecycle": { "type": "object" }, + "terminationGracePeriodSeconds": { "type": "integer", "minimum": 1 }, + "shutdownDrainSeconds": { "type": "integer", "minimum": 0 }, + "shutdownCleanupSeconds": { "type": "integer", "minimum": 0 }, "ingress": { "type": "object", "additionalProperties": false, diff --git a/charts/operator/values.yaml b/charts/operator/values.yaml index d972394f..145f8f19 100644 --- a/charts/operator/values.yaml +++ b/charts/operator/values.yaml @@ -52,6 +52,13 @@ resources: extraEnv: [] extraEnvFrom: [] +extraVolumes: [] +extraVolumeMounts: [] + +lifecycle: {} +terminationGracePeriodSeconds: 90 +shutdownDrainSeconds: 60 +shutdownCleanupSeconds: 15 ingress: enabled: false diff --git a/docs/getting-started/platforms/kubernetes.md b/docs/getting-started/platforms/kubernetes.md index 1376ed4c..cd390411 100644 --- a/docs/getting-started/platforms/kubernetes.md +++ b/docs/getting-started/platforms/kubernetes.md @@ -24,19 +24,14 @@ Operator in a kubernetes cluster is an application with a volume and a port. It ## Install -```bash -helm install operator oci://ghcr.io/untra/charts/operator \ - --namespace operator --create-namespace \ - --set publicUrl=https://operator.example.com -``` - The chart's `appVersion` is the image tag. It is pinned to an exact release - the chart never deploys `latest`. ### Bootstrap the admin account -Operator's API is [always authenticated](/security/authentication/). Before installing, create the bootstrap Secret holding a temporary password: +Operator's API is [always authenticated](/security/authentication/). Before installing, create the namespace and bootstrap Secret holding a temporary password: ```bash +kubectl create namespace operator kubectl -n operator create secret generic operator-bootstrap \ --from-literal=password="$(openssl rand -base64 24)" ``` @@ -45,7 +40,7 @@ Then reference it: ```bash helm install operator oci://ghcr.io/untra/charts/operator \ - --namespace operator --create-namespace \ + --namespace operator \ --set publicUrl=https://operator.example.com \ --set bootstrap.existingSecret=operator-bootstrap ``` @@ -158,7 +153,7 @@ Note default: with `enabled: true` and an empty `egress.to`, the rendered policy The base image ships `git`, `tmux`, `openssh-client`, `curl`, and `ca-certificates`, but **no agent CLI** - no `claude`, `codex`, or `gemini`, and no credentials for them. ```dockerfile -FROM untra/operator:0.2.7 +FROM untra/operator:{{ site.version }} USER root RUN apt-get update && apt-get install -y --no-install-recommends nodejs npm \ && npm install -g @anthropic-ai/claude-code \ @@ -169,7 +164,7 @@ USER 10001 ```yaml image: repository: registry.example.com/operator-claude - tag: "0.2.7" + tag: "{{ site.version }}" ``` Provide the agent's credentials as environment variables from a Secret: @@ -228,6 +223,37 @@ The image already ships `openssh-client`, and Operator downloads the `coder` CLI No custom image, initContainer, ConfigMap, or relaxing of `readOnlyRootFilesystem` is required - the configuration, cache, and SSH fragments all live under `/op`. +### Private certificate authorities + +When Coder or a Git forge uses an internal CA, create a ConfigMap containing a +bundle with both public and internal roots, then mount it and configure the +clients that use it: + +```yaml +extraEnv: + - name: SSL_CERT_FILE + value: /etc/operator-ca/ca-bundle.crt + - name: GIT_SSL_CAINFO + value: /etc/operator-ca/ca-bundle.crt +extraVolumes: + - name: operator-ca + configMap: + name: operator-ca +extraVolumeMounts: + - name: operator-ca + mountPath: /etc/operator-ca + readOnly: true +``` + +Mounting a certificate does not modify the image's system trust store. Verify +Operator, `coder`, `curl`, and Git separately because they can use different TLS +implementations and environment variables. + +For Git over SSH, mount a Secret containing the private key and a pinned +`known_hosts` file at a path readable by UID/GID 10001. Set `GIT_SSH_COMMAND` +to reference both paths with `StrictHostKeyChecking=yes`; do not disable host-key +verification. + ## Security context Applied by default; you should not need to change any of it: @@ -257,6 +283,14 @@ helm upgrade operator oci://ghcr.io/untra/charts/operator --reuse-values The StatefulSet uses `RollingUpdate`, but with one replica on a ReadWriteOnce volume the old pod must terminate before the new one attaches. +On SIGTERM, Operator becomes unready and stops accepting new launches. It waits +up to `shutdownDrainSeconds` for active agents, then preserves remote Coder/SSH +work for reconciliation after restart and interrupts remaining local sessions. +`shutdownCleanupSeconds` reserves time for state persistence and server cleanup. +The chart defaults the Kubernetes grace period to 90 seconds; it must be greater +than the sum of both shutdown intervals. A custom `lifecycle` hook consumes the +same grace period. + The authentication database migrates forward automatically on start. ## Backup and restore diff --git a/docs/schemas/openapi.json b/docs/schemas/openapi.json index b1bd4ce6..00329a77 100644 --- a/docs/schemas/openapi.json +++ b/docs/schemas/openapi.json @@ -5264,6 +5264,16 @@ } } } + }, + "503": { + "description": "Operator is draining", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorResponse" + } + } + } } }, "security": [ @@ -10228,6 +10238,15 @@ "name": { "type": "string" }, + "parameters": { + "type": "object", + "additionalProperties": { + "type": "string" + }, + "propertyNames": { + "type": "string" + } + }, "template": { "type": "string" } diff --git a/src/agents/launcher/mod.rs b/src/agents/launcher/mod.rs index c67c8ce2..50ac7141 100644 --- a/src/agents/launcher/mod.rs +++ b/src/agents/launcher/mod.rs @@ -1889,6 +1889,35 @@ impl Launcher { Ok(()) } + pub fn kill_local_agent_session(&self, agent: &crate::state::AgentState) -> Result<()> { + let session_name = agent + .session_name + .as_deref() + .context("Agent has no session name")?; + match agent.session_wrapper.as_deref().unwrap_or("tmux") { + "tmux" => self.kill_session(session_name), + "cmux" => { + let workspace = agent + .session_context_ref + .as_deref() + .context("cmux agent has no workspace reference")?; + self.cmux + .as_ref() + .context("cmux client is unavailable")? + .close_workspace(workspace) + .context("Failed to close cmux workspace") + } + "zellij" => self + .zellij + .as_ref() + .context("zellij client is unavailable")? + .close_tab(session_name) + .context("Failed to close zellij tab"), + "vscode" => anyhow::bail!("VS Code does not expose session termination to Operator"), + wrapper => anyhow::bail!("Unknown session wrapper '{wrapper}'"), + } + } + /// Capture the current content of a session's pane pub fn capture_session_content(&self, session_name: &str) -> Result { self.tmux diff --git a/src/config.rs b/src/config.rs index 3a157da5..031272dc 100644 --- a/src/config.rs +++ b/src/config.rs @@ -301,6 +301,12 @@ pub struct RestApiConfig { /// Externally reachable base URL (e.g. `https://operator.example.com`). Defaults to request host. #[serde(default)] pub public_url: Option, + /// Maximum time to wait for active agents before shutdown cleanup begins. + #[serde(default = "default_shutdown_drain_seconds")] + pub shutdown_drain_seconds: u64, + /// Maximum time reserved for final callbacks and persistent cleanup. + #[serde(default = "default_shutdown_cleanup_seconds")] + pub shutdown_cleanup_seconds: u64, } fn default_rest_enabled() -> bool { @@ -315,6 +321,14 @@ fn default_rest_port() -> u16 { 7008 } +fn default_shutdown_drain_seconds() -> u64 { + 60 +} + +fn default_shutdown_cleanup_seconds() -> u64 { + 15 +} + impl Default for RestApiConfig { fn default() -> Self { Self { @@ -323,6 +337,8 @@ impl Default for RestApiConfig { port: default_rest_port(), cors_origins: Vec::new(), public_url: None, + shutdown_drain_seconds: default_shutdown_drain_seconds(), + shutdown_cleanup_seconds: default_shutdown_cleanup_seconds(), } } } @@ -1014,9 +1030,13 @@ mod tests { let cfg = config_from_env(&[ ("OPERATOR_REST_API__HOST", "0.0.0.0"), ("OPERATOR_REST_API__PORT", "7099"), + ("OPERATOR_REST_API__SHUTDOWN_DRAIN_SECONDS", "45"), + ("OPERATOR_REST_API__SHUTDOWN_CLEANUP_SECONDS", "10"), ]); assert_eq!(cfg.rest_api.host, "0.0.0.0"); assert_eq!(cfg.rest_api.port, 7099); + assert_eq!(cfg.rest_api.shutdown_drain_seconds, 45); + assert_eq!(cfg.rest_api.shutdown_cleanup_seconds, 10); } #[test] diff --git a/src/rest/dto/setup.rs b/src/rest/dto/setup.rs index dfef9aba..beb29e0d 100644 --- a/src/rest/dto/setup.rs +++ b/src/rest/dto/setup.rs @@ -55,7 +55,12 @@ pub struct HostedCollectionSelection { #[ts(export)] pub enum SetupExecutionTarget { Local, - Coder { name: String, template: String }, + Coder { + name: String, + template: String, + #[serde(default)] + parameters: HashMap, + }, } #[derive(Debug, Clone, Serialize, Deserialize, ToSchema, JsonSchema, TS)] diff --git a/src/rest/error.rs b/src/rest/error.rs index 0110cbf7..8ce35110 100644 --- a/src/rest/error.rs +++ b/src/rest/error.rs @@ -25,6 +25,8 @@ pub enum ApiError { InternalError(String), /// Bad request BadRequest(String), + /// Server is unavailable for new work. + Unavailable(String), /// Cannot modify builtin resource BuiltinReadOnly(String), // The three auth variants below are constructed by the authorization @@ -59,6 +61,7 @@ impl IntoResponse for ApiError { (StatusCode::INTERNAL_SERVER_ERROR, "internal_error", msg) } ApiError::BadRequest(msg) => (StatusCode::BAD_REQUEST, "bad_request", msg), + ApiError::Unavailable(msg) => (StatusCode::SERVICE_UNAVAILABLE, "unavailable", msg), ApiError::BuiltinReadOnly(msg) => (StatusCode::FORBIDDEN, "builtin_readonly", msg), ApiError::Unauthorized(msg) => (StatusCode::UNAUTHORIZED, "unauthorized", msg), ApiError::Forbidden(msg) => (StatusCode::FORBIDDEN, "forbidden", msg), diff --git a/src/rest/mod.rs b/src/rest/mod.rs index 2acffa70..2164510f 100644 --- a/src/rest/mod.rs +++ b/src/rest/mod.rs @@ -4,6 +4,7 @@ //! and collections. Designed to run alongside the TUI or as a standalone server. use std::net::SocketAddr; +use std::time::{Duration, Instant}; use anyhow::Result; use axum::{ @@ -352,7 +353,7 @@ pub async fn serve(state: ApiState, port: u16) -> Result<()> { let tickets_path = state.tickets_path.clone(); let state_path = state.config().state_path(); let host_ip = state.config().rest_api.host_ip(); - let app = build_router(state); + let app = build_router(state.clone()); let addr = SocketAddr::new(host_ip, port); tracing::info!("REST API listening on http://{}", addr); @@ -365,7 +366,7 @@ pub async fn serve(state: ApiState, port: u16) -> Result<()> { // Serve with graceful shutdown axum::serve(listener, app) - .with_graceful_shutdown(shutdown_signal()) + .with_graceful_shutdown(shutdown_signal(state.clone())) .await?; // Clean up session file on shutdown @@ -412,7 +413,7 @@ fn remove_session_file(tickets_path: &std::path::Path) { } /// Shutdown signal handler for graceful termination -async fn shutdown_signal() { +async fn shutdown_signal(state: state::ApiState) { let ctrl_c = async { tokio::signal::ctrl_c() .await @@ -438,6 +439,121 @@ async fn shutdown_signal() { println!("\nReceived terminate signal, shutting down..."); }, } + + if !state.start_draining() { + return; + } + + let config = state.config(); + let drain_deadline = + Instant::now() + Duration::from_secs(config.rest_api.shutdown_drain_seconds); + tracing::info!( + drain_seconds = config.rest_api.shutdown_drain_seconds, + "Shutdown drain started" + ); + + loop { + let active_agents = crate::state::State::load(&config) + .map(|app_state| { + app_state.agents.iter().any(|agent| { + matches!( + agent.status.as_str(), + "running" | "awaiting_input" | "completing" + ) + }) + }) + .unwrap_or(true); + if !active_agents && state.in_flight_operations() == 0 { + break; + } + if Instant::now() >= drain_deadline { + break; + } + tokio::time::sleep(Duration::from_millis(250)).await; + } + + state.start_stopping(); + state.mcp_sessions.lock().await.clear(); + let cleanup_deadline = + Instant::now() + Duration::from_secs(config.rest_api.shutdown_cleanup_seconds); + while state.in_flight_operations() != 0 && Instant::now() < cleanup_deadline { + tokio::time::sleep(Duration::from_millis(50)).await; + } + + if state.in_flight_operations() == 0 { + let cleanup_config = (*config).clone(); + let cleanup = + tokio::task::spawn_blocking(move || interrupt_agents_for_shutdown(&cleanup_config)); + let remaining = cleanup_deadline.saturating_duration_since(Instant::now()); + match tokio::time::timeout(remaining, cleanup).await { + Ok(Ok(Ok(()))) => {} + Ok(Ok(Err(error))) => tracing::error!(%error, "Shutdown agent cleanup failed"), + Ok(Err(error)) => tracing::error!(%error, "Shutdown cleanup task failed"), + Err(_) => tracing::error!("Shutdown cleanup exceeded its deadline"), + } + } else { + tracing::error!( + in_flight = state.in_flight_operations(), + "Skipping agent state cleanup to avoid overwriting in-flight completion writes" + ); + } +} + +fn interrupt_agents_for_shutdown(config: &crate::config::Config) -> Result<()> { + use crate::state::ShutdownRecovery; + + const INTERRUPTION_MESSAGE: &str = "Interrupted by Operator shutdown; retry explicitly"; + + let mut app_state = crate::state::State::load(config)?; + let launcher = match crate::agents::Launcher::new(config) { + Ok(launcher) => Some(launcher), + Err(error) => { + tracing::warn!(%error, "Session controls are unavailable during shutdown"); + None + } + }; + for agent in &mut app_state.agents { + if !matches!( + agent.status.as_str(), + "running" | "awaiting_input" | "completing" + ) { + continue; + } + let configured_remote = agent + .target_name + .as_deref() + .and_then(|name| config.targets.iter().find(|target| target.name == name)) + .is_some_and(|target| { + matches!( + &target.kind, + crate::config::TargetKind::Coder(_) | crate::config::TargetKind::Ssh(_) + ) + }); + let remote = agent.remote_host.is_some() + || configured_remote + || agent + .launch_mode + .as_deref() + .is_some_and(|mode| mode.starts_with("coder") || mode.starts_with("ssh")); + if remote { + agent.shutdown_recovery = Some(ShutdownRecovery::RemoteAwaitingReconciliation); + agent.last_message = Some("Remote work preserved during Operator shutdown".to_string()); + continue; + } + + if agent.session_name.is_some() { + if let Some(launcher) = &launcher { + if let Err(error) = launcher.kill_local_agent_session(agent) { + tracing::warn!(%error, agent_id = %agent.id, "Failed to stop local session during shutdown"); + } + } + } + agent.status = "failed".to_string(); + agent.last_message = Some(INTERRUPTION_MESSAGE.to_string()); + agent.shutdown_recovery = Some(ShutdownRecovery::InterruptedLocal); + agent.last_activity = chrono::Utc::now(); + } + app_state.save() } #[cfg(test)] @@ -453,4 +569,60 @@ mod tests { let _router = build_router(state); // Router builds without panicking } + + #[test] + fn shutdown_marks_local_work_interrupted_and_preserves_remote_work() { + let temp = tempfile::TempDir::new().unwrap(); + let mut config = Config::default(); + config.paths.state = temp.path().join("state").display().to_string(); + config.paths.tickets = temp.path().join("tickets").display().to_string(); + config.paths.projects = temp.path().join("projects").display().to_string(); + + let mut app_state = crate::state::State::load(&config).unwrap(); + let local_id = app_state + .add_agent_with_full_options( + "LOCAL-1".to_string(), + "FEAT".to_string(), + "project".to_string(), + false, + None, + Some("default".to_string()), + None, + ) + .unwrap(); + let remote_id = app_state + .add_agent_with_full_options( + "REMOTE-1".to_string(), + "FEAT".to_string(), + "project".to_string(), + false, + None, + Some("coder".to_string()), + None, + ) + .unwrap(); + + interrupt_agents_for_shutdown(&config).unwrap(); + let app_state = crate::state::State::load(&config).unwrap(); + let local = app_state + .agents + .iter() + .find(|agent| agent.id == local_id) + .unwrap(); + assert_eq!(local.status, "failed"); + assert_eq!( + local.shutdown_recovery, + Some(crate::state::ShutdownRecovery::InterruptedLocal) + ); + let remote = app_state + .agents + .iter() + .find(|agent| agent.id == remote_id) + .unwrap(); + assert_eq!(remote.status, "running"); + assert_eq!( + remote.shutdown_recovery, + Some(crate::state::ShutdownRecovery::RemoteAwaitingReconciliation) + ); + } } diff --git a/src/rest/routes/launch.rs b/src/rest/routes/launch.rs index e3fdcec1..f405989d 100644 --- a/src/rest/routes/launch.rs +++ b/src/rest/routes/launch.rs @@ -152,6 +152,7 @@ fn prepared_launch_to_response(prepared: PreparedLaunch) -> LaunchTicketResponse (status = 200, description = "Ticket launched successfully", body = LaunchTicketResponse), (status = 404, description = "Ticket not found"), (status = 409, description = "Ticket already in progress"), + (status = 503, description = "Operator is draining"), (status = 400, description = "Invalid request") ) )] @@ -160,6 +161,22 @@ pub async fn launch_ticket( Path(ticket_id): Path, Json(request): Json, ) -> Result, ApiError> { + let _launch_permit = state.begin_launch()?; + let recovery_state = crate::state::State::load(&state.config()) + .map_err(|error| ApiError::InternalError(error.to_string()))?; + if recovery_state.agents.iter().any(|agent| { + agent.ticket_id == ticket_id + && matches!( + agent.status.as_str(), + "running" | "awaiting_input" | "completing" + ) + && agent.shutdown_recovery + == Some(crate::state::ShutdownRecovery::RemoteAwaitingReconciliation) + }) { + return Err(ApiError::Conflict(format!( + "Ticket '{ticket_id}' has remote work awaiting reconciliation" + ))); + } // Create a queue to find the ticket let queue = Queue::new(&state.config()).map_err(|e| ApiError::InternalError(e.to_string()))?; @@ -241,6 +258,25 @@ pub async fn launch_ticket( } }; + let mut recovery_state = crate::state::State::load(&state.config()) + .map_err(|error| ApiError::InternalError(error.to_string()))?; + let mut recovery_cleared = false; + for agent in recovery_state + .agents + .iter_mut() + .filter(|agent| agent.ticket_id == ticket_id) + { + if agent.shutdown_recovery == Some(crate::state::ShutdownRecovery::InterruptedLocal) { + agent.shutdown_recovery = None; + recovery_cleared = true; + } + } + if recovery_cleared { + recovery_state + .save() + .map_err(|error| ApiError::InternalError(error.to_string()))?; + } + Ok(Json(response)) } @@ -631,10 +667,8 @@ pub async fn complete_step( Authenticated(principal): Authenticated, Json(request): Json, ) -> Result, ApiError> { - // A callback token is pinned to one ticket and step. Presenting a valid but - // *different* one here would let an agent working on one ticket drive - // another ticket's workflow forward, so the claims are matched against the - // path rather than trusted for having verified at all. + let _callback_permit = state.begin_callback()?; + // A callback token is pinned to one ticket and step. The claims are matched against the path if principal.kind == PrincipalKind::AgentCallback { let matches_ticket = principal.ticket_id.as_deref() == Some(ticket_id.as_str()); let matches_step = principal.step.as_deref() == Some(step_name.as_str()); @@ -670,12 +704,7 @@ pub async fn complete_step( )) })?; - // Clone what the rest of the function needs from the registry, then drop - // the read guard before any `.await`. The proof hook below runs an - // assertion command synchronously (up to its configured timeout, default - // 120s) - holding `registry.read()` across that would stall every - // `registry.write()` caller (issuetypes/collections/steps routes) for - // the duration of each Proof-reviewed step completion. + // Clone what the rest of the function needs from the registry, then drop the read guard before any `.await`. let current_step = current_step.clone(); let next_step_schema = current_step .next_step @@ -732,7 +761,8 @@ pub async fn complete_step( }); // Determine if we should auto-proceed - let auto_proceed = status == "completed" + let auto_proceed = !state.is_draining() + && status == "completed" && next_step_info.is_some() && current_step.review_type == crate::templates::schema::ReviewType::None; diff --git a/src/rest/routes/probes.rs b/src/rest/routes/probes.rs index 19a9bd8d..b7213c24 100644 --- a/src/rest/routes/probes.rs +++ b/src/rest/routes/probes.rs @@ -43,6 +43,9 @@ pub async fn livez() -> impl IntoResponse { ) )] pub async fn readyz(State(state): State) -> impl IntoResponse { + if state.is_draining() { + return (StatusCode::SERVICE_UNAVAILABLE, "draining"); + } let store = state.auth.store.clone(); let reachable = tokio::task::spawn_blocking(move || store.bootstrap_state()) .await @@ -54,3 +57,27 @@ pub async fn readyz(State(state): State) -> impl IntoResponse { (StatusCode::SERVICE_UNAVAILABLE, "auth store unavailable") } } + +#[cfg(test)] +mod tests { + use super::*; + + #[tokio::test] + async fn readiness_closes_when_shutdown_draining_starts() { + let temp = tempfile::TempDir::new().unwrap(); + let mut config = crate::config::Config::default(); + config.paths.state = temp.path().join("state").display().to_string(); + config.paths.tickets = temp.path().join("tickets").display().to_string(); + let state = ApiState::new(config, temp.path().join("tickets")); + + assert_eq!( + readyz(State(state.clone())).await.into_response().status(), + StatusCode::OK + ); + assert!(state.start_draining()); + assert_eq!( + readyz(State(state)).await.into_response().status(), + StatusCode::SERVICE_UNAVAILABLE + ); + } +} diff --git a/src/rest/routes/setup.rs b/src/rest/routes/setup.rs index 2555cf23..1c3494b4 100644 --- a/src/rest/routes/setup.rs +++ b/src/rest/routes/setup.rs @@ -232,7 +232,11 @@ fn selected_execution_target( ) -> Result { match target { SetupExecutionTarget::Local => Ok(crate::config::TargetDef::local()), - SetupExecutionTarget::Coder { name, template } => { + SetupExecutionTarget::Coder { + name, + template, + parameters, + } => { let name = name.trim(); let template = template.trim(); if wrapper == crate::config::SessionWrapperType::Zellij { @@ -260,6 +264,7 @@ fn selected_execution_target( display_name: Some("Coder".to_string()), kind: crate::config::TargetKind::Coder(crate::config::CoderConfig { template: template.to_string(), + parameters, ..Default::default() }), }) @@ -382,12 +387,48 @@ mod tests { SetupExecutionTarget::Coder { name: "coder-agents".to_string(), template: "operator".to_string(), + parameters: std::collections::HashMap::new(), }, crate::config::SessionWrapperType::Zellij, ); assert!(matches!(result, Err(ApiError::ValidationError(_)))); } + #[test] + fn test_coder_target_preserves_template_parameters() { + let parameters = std::collections::HashMap::from([ + ("region".to_string(), "us-west".to_string()), + ("optional".to_string(), String::new()), + ]); + let target = selected_execution_target( + SetupExecutionTarget::Coder { + name: "coder-agents".to_string(), + template: "operator".to_string(), + parameters: parameters.clone(), + }, + crate::config::SessionWrapperType::Tmux, + ) + .unwrap(); + let crate::config::TargetKind::Coder(coder) = target.kind else { + panic!("expected coder target"); + }; + assert_eq!(coder.parameters, parameters); + } + + #[test] + fn test_coder_target_without_parameters_remains_compatible() { + let target: SetupExecutionTarget = serde_json::from_value(serde_json::json!({ + "kind": "coder", + "name": "coder-agents", + "template": "operator" + })) + .unwrap(); + let SetupExecutionTarget::Coder { parameters, .. } = target else { + panic!("expected coder target"); + }; + assert!(parameters.is_empty()); + } + #[tokio::test] async fn test_initialize_persists_and_rejects_reinitialization() { let temp = tempfile::TempDir::new().unwrap(); diff --git a/src/rest/state.rs b/src/rest/state.rs index d65a3622..3ae2e21c 100644 --- a/src/rest/state.rs +++ b/src/rest/state.rs @@ -2,12 +2,17 @@ use std::collections::HashMap; use std::path::PathBuf; +use std::sync::atomic::{AtomicU8, AtomicUsize, Ordering}; use std::sync::Arc; use std::sync::RwLock as StdRwLock; use tokio::sync::{Mutex, RwLock}; +const PHASE_RUNNING: u8 = 0; +const PHASE_DRAINING: u8 = 1; +const PHASE_STOPPING: u8 = 2; + use crate::api::kanban_sync::KanbanBidirectionalSync; use crate::auth::store::AuthStore; use crate::auth::tokens::SigningKey; @@ -35,6 +40,22 @@ pub struct ApiState { pub kanban_sync: Option>, /// Authentication store and signing key. pub auth: Arc, + lifecycle: Arc, +} + +struct LifecycleState { + phase: AtomicU8, + in_flight: AtomicUsize, +} + +pub struct OperationPermit { + lifecycle: Arc, +} + +impl Drop for OperationPermit { + fn drop(&mut self) { + self.lifecycle.in_flight.fetch_sub(1, Ordering::AcqRel); + } } /// An open MCP SSE session. @@ -105,6 +126,7 @@ impl ApiState { /// Build with a caller-supplied auth context, so tests and the TUI can share /// one already-open store instead of racing to open the same database file. pub fn with_auth(config: Config, tickets_path: PathBuf, auth: Arc) -> Self { + reconcile_shutdown_recovery(&config); // Shared loader; keeps the API's issue-type resolution identical to the CLI/TUI `workflow export` produces the same output on every surface. let registry = load_registry(&tickets_path); @@ -126,9 +148,79 @@ impl ApiState { mcp_sessions: Arc::new(Mutex::new(HashMap::new())), kanban_sync, auth, + lifecycle: Arc::new(LifecycleState { + phase: AtomicU8::new(PHASE_RUNNING), + in_flight: AtomicUsize::new(0), + }), } } + pub fn is_accepting_launches(&self) -> bool { + self.lifecycle.phase.load(Ordering::Acquire) == PHASE_RUNNING + } + + pub fn is_draining(&self) -> bool { + self.lifecycle.phase.load(Ordering::Acquire) != PHASE_RUNNING + } + + pub fn begin_launch(&self) -> Result { + if !self.is_accepting_launches() { + return Err(ApiError::Unavailable( + "Operator is draining and is not accepting new launches".to_string(), + )); + } + self.lifecycle.in_flight.fetch_add(1, Ordering::AcqRel); + if !self.is_accepting_launches() { + self.lifecycle.in_flight.fetch_sub(1, Ordering::AcqRel); + return Err(ApiError::Unavailable( + "Operator is draining and is not accepting new launches".to_string(), + )); + } + Ok(OperationPermit { + lifecycle: Arc::clone(&self.lifecycle), + }) + } + + pub fn begin_callback(&self) -> Result { + if self.lifecycle.phase.load(Ordering::Acquire) == PHASE_STOPPING { + return Err(ApiError::Unavailable( + "Operator is stopping and cannot accept more completion callbacks".to_string(), + )); + } + self.lifecycle.in_flight.fetch_add(1, Ordering::AcqRel); + if self.lifecycle.phase.load(Ordering::Acquire) == PHASE_STOPPING { + self.lifecycle.in_flight.fetch_sub(1, Ordering::AcqRel); + return Err(ApiError::Unavailable( + "Operator is stopping and cannot accept more completion callbacks".to_string(), + )); + } + Ok(OperationPermit { + lifecycle: Arc::clone(&self.lifecycle), + }) + } + + pub fn start_draining(&self) -> bool { + self.lifecycle + .phase + .compare_exchange( + PHASE_RUNNING, + PHASE_DRAINING, + Ordering::AcqRel, + Ordering::Acquire, + ) + .is_ok() + } + + pub fn start_stopping(&self) { + self.lifecycle + .phase + .store(PHASE_STOPPING, Ordering::Release); + } + + pub fn in_flight_operations(&self) -> usize { + self.lifecycle.in_flight.load(Ordering::Acquire) + } + /// A snapshot of the live configuration. /// /// Clones the inner `Arc` so a handler reads a consistent view for the duration of a request. @@ -173,6 +265,39 @@ impl ApiState { } } +fn reconcile_shutdown_recovery(config: &Config) { + let Ok(mut app_state) = crate::state::State::load(config) else { + return; + }; + let mut changed = false; + for agent in &mut app_state.agents { + match agent.shutdown_recovery { + Some(crate::state::ShutdownRecovery::RemoteAwaitingReconciliation) => { + if matches!( + agent.status.as_str(), + "running" | "awaiting_input" | "completing" + ) { + agent.last_message = Some( + "Remote work is awaiting reconciliation after Operator restart".to_string(), + ); + } else { + agent.shutdown_recovery = None; + } + changed = true; + } + Some(crate::state::ShutdownRecovery::InterruptedLocal) => { + agent.status = "failed".to_string(); + } + None => {} + } + } + if changed { + if let Err(error) = app_state.save() { + tracing::error!(%error, "Failed to persist shutdown recovery state"); + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -242,4 +367,30 @@ mod tests { "kanban_sync should be Some when at least one project has bidirectional: true" ); } + + #[test] + fn draining_closes_launch_admission_without_losing_in_flight_tracking() { + let temp = tempfile::TempDir::new().unwrap(); + let mut config = Config::default(); + config.paths.state = temp.path().join("state").display().to_string(); + config.paths.tickets = temp.path().join("tickets").display().to_string(); + let state = ApiState::new(config, temp.path().join("tickets")); + + let permit = state.begin_launch().unwrap(); + assert_eq!(state.in_flight_operations(), 1); + assert!(state.start_draining()); + assert!(matches!( + state.begin_launch(), + Err(ApiError::Unavailable(_)) + )); + assert!(state.begin_callback().is_ok()); + drop(permit); + assert_eq!(state.in_flight_operations(), 0); + + state.start_stopping(); + assert!(matches!( + state.begin_callback(), + Err(ApiError::Unavailable(_)) + )); + } } diff --git a/src/state.rs b/src/state.rs index fea476a0..fd16be4b 100644 --- a/src/state.rs +++ b/src/state.rs @@ -124,9 +124,20 @@ pub struct AgentState { /// Launch context fixed at launch time; `complete_step` reads it back to build subsequent step commands with the same delegator/tool/model. #[serde(default)] pub step_launch_context: Option, - /// Name of the resolved execution target this agent launched on + /// Name of the resolved execution target this agent launched on. #[serde(default)] pub target_name: Option, + /// Shutdown recovery strategy. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub shutdown_recovery: Option, +} + +#[derive(Debug, Clone, Copy, Serialize, Deserialize, JsonSchema, TS, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +#[ts(export)] +pub enum ShutdownRecovery { + InterruptedLocal, + RemoteAwaitingReconciliation, } #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, TS)] @@ -345,6 +356,7 @@ impl State { remote_host: None, step_launch_context: None, target_name: None, + shutdown_recovery: None, }); self.save()?; @@ -403,6 +415,7 @@ impl State { remote_host: None, step_launch_context: None, target_name: None, + shutdown_recovery: None, }); self.save()?; diff --git a/src/ui/in_progress_panel.rs b/src/ui/in_progress_panel.rs index e6faf799..8c553618 100644 --- a/src/ui/in_progress_panel.rs +++ b/src/ui/in_progress_panel.rs @@ -375,6 +375,7 @@ mod tests { remote_host: None, step_launch_context: None, target_name: None, + shutdown_recovery: None, } } diff --git a/src/ui/session_preview.rs b/src/ui/session_preview.rs index 02c8c4bf..f108a01b 100644 --- a/src/ui/session_preview.rs +++ b/src/ui/session_preview.rs @@ -348,6 +348,7 @@ mod tests { remote_host: None, step_launch_context: None, target_name: None, + shutdown_recovery: None, session_wrapper: None, session_window_ref: None, session_context_ref: None, diff --git a/tests/distribution_bundling.rs b/tests/distribution_bundling.rs index 368f6781..7820145c 100644 --- a/tests/distribution_bundling.rs +++ b/tests/distribution_bundling.rs @@ -101,11 +101,12 @@ fn test_docker_ci_job_stages_opr8r_artifacts() { let docker_job = &content[docker_job_start..]; assert!( - docker_job.contains("opr8r-linux-*"), - "the docker job must download opr8r-linux-* release artifacts, like it does for operator-linux-*" + docker_job.contains("opr8r_artifact: opr8r-linux-x86_64") + && docker_job.contains("opr8r_artifact: opr8r-linux-arm64"), + "the docker matrix must download the opr8r artifact for both architectures" ); assert!( - docker_job.contains("opr8r-linux-amd64") && docker_job.contains("opr8r-linux-arm64"), - "the docker job must stage opr8r-linux-amd64/arm64 into the build context, like it does for operator" + docker_job.contains("opr8r-linux-${{ matrix.arch }}"), + "the docker job must stage opr8r under the Dockerfile's TARGETARCH naming convention" ); } diff --git a/ui/src/routes/onboarding/OnboardingPage.module.css b/ui/src/routes/onboarding/OnboardingPage.module.css index 6e72c66f..062cade0 100644 --- a/ui/src/routes/onboarding/OnboardingPage.module.css +++ b/ui/src/routes/onboarding/OnboardingPage.module.css @@ -25,6 +25,9 @@ .form label, .mapping label { display: grid; gap: .35rem; } .form input, .form select, .mapping select, .editor { box-sizing: border-box; width: 100%; padding: .65rem; color: inherit; background: var(--color-bg); border: 1px solid var(--border); border-radius: .25rem; } .form button, .content footer button, .export button { padding: .65rem 1rem; color: inherit; background: var(--surface); border: 1px solid var(--border); border-radius: .25rem; cursor: pointer; } +.parameters { display: grid; gap: .75rem; padding: 1rem; border: 1px solid var(--border); border-radius: .25rem; } +.parameterRow { display: grid; grid-template-columns: 1fr 1fr auto; gap: .5rem; } +.parameterRow input { min-width: 0; } .mapping { display: grid; grid-template-columns: repeat(3, 1fr); gap: .75rem; } .editor { min-height: 24rem; resize: vertical; font-family: var(--font-mono); } .export { margin-top: 1rem; padding: 1rem; border: 1px solid var(--accent); border-radius: .4rem; } @@ -33,4 +36,4 @@ .content footer .primary { color: var(--color-bg); background: var(--accent); border-color: var(--accent); } .error { width: min(54rem, calc(100% - 4rem)); margin: 1rem auto 0; padding: .75rem 1rem; color: var(--danger); border: 1px solid var(--danger); border-radius: .35rem; } .loading { min-height: 100vh; display: grid; place-items: center; background: var(--color-bg); color: var(--text); } -@media (max-width: 760px) { .page { grid-template-columns: 1fr; } .sidebar { display: none; } .content > header, .body, .content > footer, .error { width: calc(100% - 2rem); } .mapping { grid-template-columns: 1fr; } } +@media (max-width: 760px) { .page { grid-template-columns: 1fr; } .sidebar { display: none; } .content > header, .body, .content > footer, .error { width: calc(100% - 2rem); } .mapping, .parameterRow { grid-template-columns: 1fr; } } diff --git a/ui/src/routes/onboarding/OnboardingPage.tsx b/ui/src/routes/onboarding/OnboardingPage.tsx index 14499b34..23965b80 100644 --- a/ui/src/routes/onboarding/OnboardingPage.tsx +++ b/ui/src/routes/onboarding/OnboardingPage.tsx @@ -25,6 +25,7 @@ export function OnboardingPage() { taskFields: ["priority", "points", "user_story"], wrapper: "tmux", executionTarget: { kind: "local" }, + coderParameters: [], useWorktrees: false, acceptanceCriteria: "", modelServers: [], @@ -61,6 +62,7 @@ export function OnboardingPage() { taskFields: ["priority", "points", "user_story"], wrapper: "tmux", executionTarget: { kind: "local" }, + coderParameters: [], useWorktrees: false, acceptanceCriteria: nextStatus.default_acceptance_criteria, modelServers: [], @@ -111,6 +113,17 @@ export function OnboardingPage() { setError("Coder target name and template are required."); return; } + if (current.slug === "execution-target" && draft.executionTarget.kind === "coder") { + const names = draft.coderParameters.map((parameter) => parameter.name.trim()); + if (names.some((name) => !name.trim())) { + setError("Coder parameter names cannot be empty."); + return; + } + if (new Set(names).size !== names.length) { + setError("Coder parameter names must be unique."); + return; + } + } setError(null); setCurrentSlug(walk[Math.min(currentIndex + 1, walk.length - 1)]); } @@ -119,11 +132,20 @@ export function OnboardingPage() { setBusy(true); setError(null); try { + const executionTarget: WizardDraft["executionTarget"] = + draft.executionTarget.kind === "coder" + ? { + ...draft.executionTarget, + parameters: Object.fromEntries( + draft.coderParameters.map(({ name, value }) => [name.trim(), value]), + ), + } + : draft.executionTarget; await api.initializeSetup({ preset: draft.preset, task_fields: draft.taskFields, wrapper: draft.wrapper, - execution_target: draft.executionTarget, + execution_target: executionTarget, use_worktrees: draft.executionTarget.kind === "coder" ? false : draft.useWorktrees, acceptance_criteria: draft.acceptanceCriteria, model_servers: draft.modelServers, diff --git a/ui/src/routes/onboarding/steps.tsx b/ui/src/routes/onboarding/steps.tsx index 28e7efe9..63ef332a 100644 --- a/ui/src/routes/onboarding/steps.tsx +++ b/ui/src/routes/onboarding/steps.tsx @@ -628,7 +628,12 @@ const ExecutionTarget: StepComponent = ({ draft, setDraft }) => ( setDraft((current) => ({ ...current, useWorktrees: false, - executionTarget: { kind: "coder", name: "coder-agents", template: "" }, + executionTarget: { + kind: "coder", + name: "coder-agents", + template: "", + parameters: {}, + }, })) } > @@ -652,6 +657,7 @@ const ExecutionTarget: StepComponent = ({ draft, setDraft }) => ( current.executionTarget.kind === "coder" ? current.executionTarget.template : "", + parameters: {}, }, })) } @@ -671,11 +677,80 @@ const ExecutionTarget: StepComponent = ({ draft, setDraft }) => ( ? current.executionTarget.name : "coder-agents", template: event.target.value, + parameters: {}, }, })) } /> +
+ Template parameters (optional) + {draft.coderParameters.map((parameter, index) => ( +
+ + setDraft((current) => ({ + ...current, + coderParameters: current.coderParameters.map((item) => + item.id === parameter.id ? { ...item, name: event.target.value } : item, + ), + })) + } + /> + + setDraft((current) => ({ + ...current, + coderParameters: current.coderParameters.map((item) => + item.id === parameter.id ? { ...item, value: event.target.value } : item, + ), + })) + } + /> + +
+ ))} + +

Set CODER_URL and CODER_SESSION_TOKEN in the server environment.

)} diff --git a/ui/src/routes/onboarding/types.ts b/ui/src/routes/onboarding/types.ts index c34a34af..8f12447b 100644 --- a/ui/src/routes/onboarding/types.ts +++ b/ui/src/routes/onboarding/types.ts @@ -14,6 +14,7 @@ export type WizardDraft = { taskFields: string[]; wrapper: SessionWrapperType; executionTarget: SetupExecutionTarget; + coderParameters: Array<{ id: number; name: string; value: string }>; useWorktrees: boolean; acceptanceCriteria: string; modelServers: string[]; From 58c4d4c4575ffa659896c787e33b042d832831a0 Mon Sep 17 00:00:00 2001 From: untra Date: Mon, 14 Sep 2026 13:26:58 -0600 Subject: [PATCH 2/4] nice, cleaner --- .github/workflows/build.yaml | 2 +- .oxlintrc.jsonc | 3 +- Dockerfile | 4 +- bindings/AgentState.ts | 1 - bindings/RestApiConfig.ts | 4 +- docs/configuration/index.md | 2 + docs/getting-started/platforms/kubernetes.md | 16 + src/agents/launcher/coder.rs | 208 ++++-- src/agents/launcher/mod.rs | 1 + src/agents/launcher/process.rs | 151 +++++ src/agents/launcher/remote.rs | 17 +- src/config.rs | 8 +- src/main.rs | 7 + src/mcp/tools.rs | 37 +- src/rest/error.rs | 15 +- src/rest/mod.rs | 196 ++++-- src/rest/routes/launch.rs | 139 +++- src/rest/state.rs | 146 ++++- src/startup/mod.rs | 1 + src/startup/recovery.rs | 195 ++++++ src/state.rs | 192 +++++- tests/distribution_bundling.rs | 81 +++ tests/helm_chart.rs | 225 +++++++ tests/setup_parity.rs | 73 +++ ui/src/Layout.tsx | 12 +- ui/src/components/KanbanBoard.tsx | 10 +- ui/src/right-panel.tsx | 13 +- ui/src/routes/DevicePage.tsx | 2 +- ui/src/routes/ForgotPasswordPage.tsx | 1 - ui/src/routes/LoginPage.tsx | 4 +- ui/src/routes/ModelProvidersPage.tsx | 52 +- ui/src/routes/onboarding/OnboardingPage.tsx | 15 +- ui/src/routes/onboarding/steps.tsx | 605 ++++++++++-------- ui/src/theme.ts | 2 +- vscode-extension/src/webhook-server.ts | 2 +- vscode-extension/webview-ui/App.tsx | 118 ++-- .../webview-ui/components/ConfigPage.tsx | 11 +- .../webview-ui/components/SidebarNav.tsx | 25 +- .../components/kanban/MappingRow.tsx | 11 +- .../components/kanban/ProjectRow.tsx | 116 ++-- .../components/kanban/ProviderCard.tsx | 91 ++- .../sections/CodingAgentsSection.tsx | 42 +- .../sections/GitRepositoriesSection.tsx | 37 +- .../sections/KanbanProvidersSection.tsx | 12 +- .../sections/ModelProvidersSection.tsx | 63 +- .../sections/PrimaryConfigSection.tsx | 23 +- vscode-extension/webview-ui/types/defaults.ts | 2 + 47 files changed, 2300 insertions(+), 693 deletions(-) create mode 100644 src/agents/launcher/process.rs create mode 100644 src/startup/recovery.rs create mode 100644 tests/helm_chart.rs diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index ae994ce4..b9a51e96 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -604,7 +604,7 @@ jobs: - name: Update TypeScript VERSION constant run: | - sed -i "s/const VERSION = '[^']*'/const VERSION = '${{ needs.version.outputs.version }}'/" vscode-extension/src/webhook-server.ts + sed -i "s/const VERSION = [\"'][^\"']*[\"']/const VERSION = \\"${{ needs.version.outputs.version }}\\"/" vscode-extension/src/webhook-server.ts # Pinned by tests/version_parity.rs; the range keeps the sed inside the # install_version block so sibling variable defaults are untouched. diff --git a/.oxlintrc.jsonc b/.oxlintrc.jsonc index c681251a..80ca2f4b 100644 --- a/.oxlintrc.jsonc +++ b/.oxlintrc.jsonc @@ -176,10 +176,9 @@ "typescript/no-unsafe-member-access": "off", "typescript/no-unsafe-return": "off", - // ---- react-perf: new class of finding, land as warn ---- "react-perf/jsx-no-new-object-as-prop": "warn", "react-perf/jsx-no-new-array-as-prop": "warn", - "react-perf/jsx-no-new-function-as-prop": "warn", + "react-perf/jsx-no-new-function-as-prop": ["error", { "nativeAllowList": "all" }], "react-perf/jsx-no-jsx-as-prop": "warn" }, diff --git a/Dockerfile b/Dockerfile index f7834ebd..5e43aea4 100644 --- a/Dockerfile +++ b/Dockerfile @@ -10,7 +10,9 @@ LABEL org.opencontainers.image.title="Operator" \ # Populated automatically by buildx per target platform (amd64 / arm64). ARG TARGETARCH -# Substrate Operator needs to launch agents: git (VCS ops), tmux (session wrapper), ca-certificates (TLS to LLM/kanban APIs), openssh-client, curl, etc. +# Substrate Operator needs to launch agents: git (VCS ops), tmux (session +# wrapper), ca-certificates (TLS to LLM/kanban APIs), openssh-client (every +# ssh and coder target launch, and git over SSH remotes), curl (in-pod reachability checks). # The LLM CLI (claude / codex / gemini) and its auth are supplied by the user via a derived image or env vars RUN apt-get update \ && apt-get upgrade -y --no-install-recommends \ diff --git a/bindings/AgentState.ts b/bindings/AgentState.ts index b021d0d8..a22c2c2e 100644 --- a/bindings/AgentState.ts +++ b/bindings/AgentState.ts @@ -74,7 +74,6 @@ llm_tool: string | null, llm_model: string | null, /** * Launch mode: `default|yolo|docker[-yolo]|coder[-yolo]|ssh[-yolo]` - * (derived from the resolved execution target; parse with `agents::parse_launch_mode`, never substring-match) */ launch_mode: string | null, /** diff --git a/bindings/RestApiConfig.ts b/bindings/RestApiConfig.ts index c06dfad4..e5ea0214 100644 --- a/bindings/RestApiConfig.ts +++ b/bindings/RestApiConfig.ts @@ -27,8 +27,8 @@ public_url: string | null, /** * Maximum time to wait for active agents before shutdown cleanup begins. */ -shutdown_drain_seconds: bigint, +shutdown_drain_seconds: number, /** * Maximum time reserved for final callbacks and persistent cleanup. */ -shutdown_cleanup_seconds: bigint, }; +shutdown_cleanup_seconds: number, }; diff --git a/docs/configuration/index.md b/docs/configuration/index.md index fa1a5e57..896f05b0 100644 --- a/docs/configuration/index.md +++ b/docs/configuration/index.md @@ -275,6 +275,8 @@ enabled = true host = "127.0.0.1" port = 7008 cors_origins = [] +shutdown_drain_seconds = 60 +shutdown_cleanup_seconds = 15 [git] branch_format = "{type}/{ticket_id}" diff --git a/docs/getting-started/platforms/kubernetes.md b/docs/getting-started/platforms/kubernetes.md index cd390411..2aa77b2d 100644 --- a/docs/getting-started/platforms/kubernetes.md +++ b/docs/getting-started/platforms/kubernetes.md @@ -291,6 +291,22 @@ The chart defaults the Kubernetes grace period to 90 seconds; it must be greater than the sum of both shutdown intervals. A custom `lifecycle` hook consumes the same grace period. +SIGTERM is the supported drain trigger; there is no HTTP drain endpoint. During +the drain window `/readyz` returns 503 while `/livez` keeps succeeding, so the +kubelet stops routing new traffic without restarting the pod. Launch requests +are refused with 503, but in-flight agent completion callbacks and status reads +keep working: a step that finishes mid-drain is still recorded, it simply does +not start the next one. + +On restart, agents that were interrupted locally come back as failed with an +explicit shutdown-interruption reason and are available for retry. Remote Coder +and SSH work is left running - Operator never stops a remote workspace just +because it is shutting down - and comes back marked as awaiting reconciliation. +Operator does not probe the remote host to resolve that state: a relaunch of the +same ticket is refused with 409 until you decide whether the remote work +survived. Remote computation can outlive Operator while its callback path is +down, so reconciliation surfaces that gap rather than guessing at it. + The authentication database migrates forward automatically on start. ## Backup and restore diff --git a/src/agents/launcher/coder.rs b/src/agents/launcher/coder.rs index ed60ee57..656a4932 100644 --- a/src/agents/launcher/coder.rs +++ b/src/agents/launcher/coder.rs @@ -184,17 +184,33 @@ pub(crate) fn checkout_script(workdir: &str, remote_url: &str, branch: &str) -> ) } +/// Budget for read-only `coder` queries. Creation and stop reuse the target's +/// own `create_timeout_secs`, which operators already tune per deployment. +const QUERY_TIMEOUT_SECS: u64 = 60; + +/// Budget for a single ssh readiness probe inside the `wait_for_ssh` loop. +const SSH_PROBE_TIMEOUT_SECS: u64 = 30; + /// Run a `coder` CLI invocation with the session injected under the CLI's /// standard env names. Errors surface Coder's stderr verbatim - quota and /// permission failures are the control plane's message, not ours to /// reinterpret. -fn run_coder(coder_bin: &Path, session: &CoderSession, args: &[&str]) -> Result { - let output = Command::new(coder_bin) +fn run_coder( + coder_bin: &Path, + session: &CoderSession, + args: &[&str], + timeout_secs: u64, +) -> Result { + let mut command = Command::new(coder_bin); + command .args(args) .env("CODER_URL", &session.url) - .env("CODER_SESSION_TOKEN", &session.token) - .output() - .context("Failed to run the `coder` CLI")?; + .env("CODER_SESSION_TOKEN", &session.token); + let output = super::process::output_with_timeout( + command, + std::time::Duration::from_secs(timeout_secs), + &format!("`coder {}`", args.join(" ")), + )?; if !output.status.success() { anyhow::bail!( "`coder {}` failed: {}", @@ -210,6 +226,7 @@ fn find_workspace( coder_bin: &Path, session: &CoderSession, name: &str, + timeout_secs: u64, ) -> Result> { let out = run_coder( coder_bin, @@ -221,6 +238,7 @@ fn find_workspace( "--output", "json", ], + timeout_secs, )?; let all: Vec = serde_json::from_str(out.trim()).unwrap_or_default(); Ok(all.into_iter().find(|w| w.name == name)) @@ -273,9 +291,7 @@ fn coder_download_url(base: &str, arch: &str) -> Result { )) } -/// Fetch the CLI from the deployment into `dest`. Downloads to a sibling -/// temp file and renames, so a killed process can never leave a truncated -/// binary that later looks like a valid cache hit. +/// Fetch the CLI from the deployment into `dest`. Downloads to a sibling temp file and renames fn download_coder_cli(base_url: &str, dest: &Path) -> Result { let url = coder_download_url(base_url, std::env::consts::ARCH)?; let dir = dest @@ -332,6 +348,25 @@ fn ensure_coder_cli(config: &Config, session: &CoderSession) -> Result } } +/// Argv for `coder create`. Parameters are sorted so a given config always +/// produces the same command, and each pair gets its own `--parameter` flag. +fn create_workspace_args(workspace: &str, coder: &CoderConfig) -> Vec { + let mut args = vec![ + "create".to_string(), + workspace.to_string(), + "--template".to_string(), + coder.template.clone(), + "-y".to_string(), + ]; + let mut params: Vec<_> = coder.parameters.iter().collect(); + params.sort(); + for (key, value) in params { + args.push("--parameter".to_string()); + args.push(format!("{key}={value}")); + } + args +} + /// Provision the workspace for a ticket and return the `RemoteHost` the /// shared remote launch tail consumes. Blocking - workspace creation is /// bounded by `create_timeout_secs`. @@ -359,7 +394,7 @@ pub(crate) fn provision_workspace( let workspace = workspace_name(&coder.name_prefix, project, ticket_id); match decide_workspace_action( - find_workspace(&coder_bin, &session, &workspace)?.as_ref(), + find_workspace(&coder_bin, &session, &workspace, QUERY_TIMEOUT_SECS)?.as_ref(), &coder.template, ) { WorkspaceAction::Refuse { existing_template } => anyhow::bail!( @@ -368,24 +403,18 @@ pub(crate) fn provision_workspace( coder.template ), WorkspaceAction::Start => { - run_coder(&coder_bin, &session, &["start", &workspace, "--no-wait"]).map(|_| ())?; + run_coder( + &coder_bin, + &session, + &["start", &workspace, "--no-wait"], + coder.create_timeout_secs, + ) + .map(|_| ())?; } WorkspaceAction::Create => { - let mut args: Vec = vec![ - "create".to_string(), - workspace.clone(), - "--template".to_string(), - coder.template.clone(), - "-y".to_string(), - ]; - let mut params: Vec<_> = coder.parameters.iter().collect(); - params.sort(); - for (k, v) in params { - args.push("--parameter".to_string()); - args.push(format!("{k}={v}")); - } + let args = create_workspace_args(&workspace, coder); let arg_refs: Vec<&str> = args.iter().map(String::as_str).collect(); - run_coder(&coder_bin, &session, &arg_refs).map(|_| ())?; + run_coder(&coder_bin, &session, &arg_refs, coder.create_timeout_secs).map(|_| ())?; } } @@ -416,8 +445,14 @@ pub(crate) fn provision_workspace( let path = super::prompt::shell_escape(&runtime.path.to_string_lossy()); script = format!(". {path}/env.sh\ntrap 'rm -rf -- {path}' EXIT\n{script}"); } - run_ssh(&session, &fragment, &alias, &script) - .with_context(|| format!("Failed to prepare checkout on workspace '{workspace}'"))?; + run_ssh( + &session, + &fragment, + &alias, + &script, + coder.create_timeout_secs, + ) + .with_context(|| format!("Failed to prepare checkout on workspace '{workspace}'"))?; } Ok(RemoteHost { name: workspace.clone(), @@ -428,18 +463,21 @@ pub(crate) fn provision_workspace( }) } -/// Stop the workspace (never delete - reclamation is the Coder admin's -/// autostop/autodelete policy). Best-effort by design. +/// Stop the workspace. Best-effort by design. pub fn stop_workspace(config: &Config, coder: &CoderConfig, workspace: &str) -> Result<()> { let session = resolve_session(coder)?; let coder_bin = ensure_coder_cli(config, &session)?; - run_coder(&coder_bin, &session, &["stop", workspace, "--yes"]).map(|_| ()) + run_coder( + &coder_bin, + &session, + &["stop", workspace, "--yes"], + coder.create_timeout_secs, + ) + .map(|_| ()) } -/// `ssh` spawns the fragment's `ProxyCommand` itself, and that `coder` -/// subprocess reads the CLI's own canonical variable names. Inject them here -/// so a target configured with custom `url_env` / `token_env` names still -/// authenticates -- in-process only, never written to the fragment on disk. +/// `ssh` spawns the fragment's `ProxyCommand` itself, and that `coder` subprocess reads the CLI's own canonical variable names. +/// Inject them here so a target configured with custom `url_env` / `token_env` names still authenticates. fn coder_ssh_command(session: &CoderSession, fragment: &Path) -> Command { let mut command = Command::new("ssh"); command @@ -459,13 +497,19 @@ fn wait_for_ssh( ) -> Result<()> { let deadline = std::time::Instant::now() + std::time::Duration::from_secs(timeout_secs); loop { - let ok = coder_ssh_command(session, fragment) + let mut probe = coder_ssh_command(session, fragment); + probe .args(["-o", "BatchMode=yes", "-o", "ConnectTimeout=10"]) .arg(alias) - .arg("true") - .status() - .map(|s| s.success()) - .unwrap_or(false); + .arg("true"); + // ConnectTimeout only bounds the handshake; a session that connects and + // then stalls would otherwise outlive the loop deadline below. + let ok = super::process::output_with_timeout( + probe, + std::time::Duration::from_secs(SSH_PROBE_TIMEOUT_SECS), + "ssh workspace readiness probe", + ) + .is_ok_and(|out| out.status.success()); if ok { return Ok(()); } @@ -479,15 +523,26 @@ fn wait_for_ssh( } } -fn run_ssh(session: &CoderSession, fragment: &Path, alias: &str, script: &str) -> Result<()> { - let status = coder_ssh_command(session, fragment) - .args(["-o", "BatchMode=yes"]) - .arg(alias) - .arg(script) - .status() - .context("Failed to run ssh against the workspace")?; - if !status.success() { - anyhow::bail!("ssh command on workspace '{alias}' exited with {status}"); +fn run_ssh( + session: &CoderSession, + fragment: &Path, + alias: &str, + script: &str, + timeout_secs: u64, +) -> Result<()> { + let mut command = coder_ssh_command(session, fragment); + command.args(["-o", "BatchMode=yes"]).arg(alias).arg(script); + let output = super::process::output_with_timeout( + command, + std::time::Duration::from_secs(timeout_secs), + &format!("ssh setup command on workspace '{alias}'"), + )?; + if !output.status.success() { + anyhow::bail!( + "ssh command on workspace '{alias}' exited with {}: {}", + output.status, + String::from_utf8_lossy(&output.stderr).trim() + ); } Ok(()) } @@ -541,6 +596,61 @@ pub fn stop_on_complete_for_agent(config: &Config, agent: &crate::state::AgentSt mod tests { use super::*; + fn coder_with_parameters(pairs: &[(&str, &str)]) -> CoderConfig { + CoderConfig { + template: "operator".to_string(), + parameters: pairs + .iter() + .map(|(k, v)| ((*k).to_string(), (*v).to_string())) + .collect(), + ..Default::default() + } + } + + #[test] + fn test_create_workspace_args_without_parameters() { + assert_eq!( + create_workspace_args("op-proj-feat-1", &coder_with_parameters(&[])), + ["create", "op-proj-feat-1", "--template", "operator", "-y"] + ); + } + + /// One flag per pair, sorted by name, so the same config always produces + /// the same command regardless of map iteration order. + #[test] + fn test_create_workspace_args_emit_sorted_separate_parameter_flags() { + let coder = coder_with_parameters(&[("region", "us-west"), ("cpu", "4")]); + assert_eq!( + create_workspace_args("op-proj-feat-1", &coder), + [ + "create", + "op-proj-feat-1", + "--template", + "operator", + "-y", + "--parameter", + "cpu=4", + "--parameter", + "region=us-west", + ] + ); + } + + /// An empty value is a meaningful Coder input (it selects a template + /// default), so it must survive as `key=` rather than being dropped. + #[test] + fn test_create_workspace_args_preserve_empty_parameter_values() { + let coder = coder_with_parameters(&[("optional", ""), ("spaced", "a b")]); + let args = create_workspace_args("ws", &coder); + assert!(args.contains(&"optional=".to_string())); + assert!(args.contains(&"spaced=a b".to_string())); + assert_eq!( + args.iter().filter(|arg| *arg == "--parameter").count(), + 2, + "each parameter needs its own flag" + ); + } + #[test] fn test_workspace_name_deterministic_and_sanitized() { let a = workspace_name("op", "MyProj", "FEAT-42"); @@ -765,10 +875,6 @@ mod tests { let script = checkout_script("/home/coder/proj", "git@github.com:u/r.git", "feat/x-42"); assert!(script.contains("git clone 'git@github.com:u/r.git'")); assert!(script.contains("fetch origin")); - assert!( - script.contains("checkout -B 'feat/x-42'"), - "branch name comes from Rust, never a template: {script}" - ); assert!(script.starts_with("set -e\n")); } diff --git a/src/agents/launcher/mod.rs b/src/agents/launcher/mod.rs index 50ac7141..61246595 100644 --- a/src/agents/launcher/mod.rs +++ b/src/agents/launcher/mod.rs @@ -10,6 +10,7 @@ pub(crate) mod coder; pub mod interpolation; pub(crate) mod llm_command; mod options; +pub(crate) mod process; pub(crate) mod prompt; pub(crate) mod remote; pub(crate) mod step_command; diff --git a/src/agents/launcher/process.rs b/src/agents/launcher/process.rs new file mode 100644 index 00000000..a49a61e5 --- /dev/null +++ b/src/agents/launcher/process.rs @@ -0,0 +1,151 @@ +//! Deadline-bounded subprocess execution for the launch path. +//! +//! `coder create` and the `ssh` invocations around a remote launch are blocking +//! and previously had no upper bound: a wedged control plane or a half-open +//! connection parked a launch forever, and dropping the HTTP request that +//! started it did not reclaim anything. Every launch-reachable command goes +//! through here so a hang fails the launch instead of leaking a worker. + +use std::io::Read; +use std::process::{Child, Command, Output, Stdio}; +use std::time::{Duration, Instant}; + +/// How often to check whether the child has exited. +const POLL_INTERVAL: Duration = Duration::from_millis(25); + +/// Run `command` to completion, killing it if it outlives `timeout`. +pub fn output_with_timeout( + mut command: Command, + timeout: Duration, + description: &str, +) -> anyhow::Result { + let mut child = command + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .map_err(|error| anyhow::anyhow!("Failed to start {description}: {error}"))?; + + let stdout = drain(child.stdout.take()); + let stderr = drain(child.stderr.take()); + + let status = match wait_with_timeout(&mut child, timeout) { + Some(status) => status, + None => { + let _ = child.kill(); + let _ = child.wait(); + anyhow::bail!( + "{description} exceeded its {}s timeout and was terminated", + timeout.as_secs() + ); + } + }; + + Ok(Output { + status, + stdout: stdout.join().unwrap_or_default(), + stderr: stderr.join().unwrap_or_default(), + }) +} + +/// `None` once the deadline passes without the child exiting. +fn wait_with_timeout(child: &mut Child, timeout: Duration) -> Option { + let deadline = Instant::now() + timeout; + loop { + match child.try_wait() { + Ok(Some(status)) => return Some(status), + Ok(None) => {} + Err(_) => return None, + } + if Instant::now() >= deadline { + return None; + } + std::thread::sleep(POLL_INTERVAL); + } +} + +fn drain(stream: Option) -> std::thread::JoinHandle> { + std::thread::spawn(move || { + let mut buffer = Vec::new(); + if let Some(mut stream) = stream { + let _ = stream.read_to_end(&mut buffer); + } + buffer + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn sh(script: &str) -> Command { + let mut command = Command::new("sh"); + command.args(["-c", script]); + command + } + + #[test] + fn test_fast_command_returns_its_output() { + let out = + output_with_timeout(sh("printf ok"), Duration::from_secs(5), "test command").unwrap(); + assert!(out.status.success()); + assert_eq!(String::from_utf8_lossy(&out.stdout), "ok"); + } + + #[test] + fn test_failing_command_surfaces_status_and_stderr() { + let out = output_with_timeout( + sh("printf boom >&2; exit 3"), + Duration::from_secs(5), + "test command", + ) + .unwrap(); + assert_eq!(out.status.code(), Some(3)); + assert_eq!(String::from_utf8_lossy(&out.stderr), "boom"); + } + + /// The defect this module exists for: without a deadline this never returns. + #[test] + fn test_hung_command_is_terminated_at_the_deadline() { + let started = Instant::now(); + let error = output_with_timeout( + sh("sleep 30"), + Duration::from_millis(200), + "wedged test command", + ) + .unwrap_err(); + + assert!( + error.to_string().contains("exceeded its"), + "unexpected error: {error}" + ); + assert!( + started.elapsed() < Duration::from_secs(5), + "the deadline must fire long before the child would exit on its own" + ); + } + + /// A child that outproduces the pipe buffer must not deadlock the poll loop. + #[test] + fn test_large_output_does_not_deadlock_the_deadline() { + let out = output_with_timeout( + sh("for i in $(seq 1 5000); do echo 0123456789012345678901234567890123456789; done"), + Duration::from_secs(20), + "chatty test command", + ) + .unwrap(); + assert!(out.status.success()); + assert!(out.stdout.len() > 200_000, "got {} bytes", out.stdout.len()); + } + + #[test] + fn test_missing_binary_names_the_operation() { + let error = output_with_timeout( + Command::new("operator-no-such-binary-xyz"), + Duration::from_secs(5), + "preflight probe", + ) + .unwrap_err(); + assert!(error.to_string().contains("preflight probe")); + } +} diff --git a/src/agents/launcher/remote.rs b/src/agents/launcher/remote.rs index 6d361e25..5d8c333e 100644 --- a/src/agents/launcher/remote.rs +++ b/src/agents/launcher/remote.rs @@ -427,6 +427,9 @@ fn preflight_script( checks } +/// Budget for the whole preflight probe, handshake included. +const PREFLIGHT_TIMEOUT_SECS: u64 = 60; + /// Check the remote host can run the agent before any session is created: /// reachable over SSH (`BatchMode` so a password prompt can't wedge the TUI), /// tmux and the tool on the remote PATH, and the workdir present. @@ -439,12 +442,16 @@ pub(crate) fn run_preflight( if let Some(ref frag) = host.ssh_config_path { cmd.args(["-F", frag]); } - let status = cmd - .args(["-o", "BatchMode=yes", "-o", "ConnectTimeout=5"]) + cmd.args(["-o", "BatchMode=yes", "-o", "ConnectTimeout=5"]) .arg(&host.ssh_alias) - .arg(preflight_script(host, tool_name, provider)) - .status() - .context("Failed to run ssh for remote preflight")?; + .arg(preflight_script(host, tool_name, provider)); + // ConnectTimeout bounds only the handshake + let status = super::process::output_with_timeout( + cmd, + std::time::Duration::from_secs(PREFLIGHT_TIMEOUT_SECS), + "remote preflight over ssh", + )? + .status; match status.code() { Some(0) => Ok(()), diff --git a/src/config.rs b/src/config.rs index 031272dc..f58b52f6 100644 --- a/src/config.rs +++ b/src/config.rs @@ -303,10 +303,10 @@ pub struct RestApiConfig { pub public_url: Option, /// Maximum time to wait for active agents before shutdown cleanup begins. #[serde(default = "default_shutdown_drain_seconds")] - pub shutdown_drain_seconds: u64, + pub shutdown_drain_seconds: u32, /// Maximum time reserved for final callbacks and persistent cleanup. #[serde(default = "default_shutdown_cleanup_seconds")] - pub shutdown_cleanup_seconds: u64, + pub shutdown_cleanup_seconds: u32, } fn default_rest_enabled() -> bool { @@ -321,11 +321,11 @@ fn default_rest_port() -> u16 { 7008 } -fn default_shutdown_drain_seconds() -> u64 { +fn default_shutdown_drain_seconds() -> u32 { 60 } -fn default_shutdown_cleanup_seconds() -> u64 { +fn default_shutdown_cleanup_seconds() -> u32 { 15 } diff --git a/src/main.rs b/src/main.rs index a103dde1..f5c52a97 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1107,6 +1107,13 @@ async fn cmd_api(config: &Config, port: Option, open: bool) -> Result<()> { }); } + // Settle what the last shutdown left behind before the API can admit work + match startup::recovery::reconcile(&config) { + Ok(0) => {} + Ok(count) => println!("Reconciled {count} agent(s) interrupted by a previous shutdown"), + Err(error) => eprintln!("Warning: shutdown recovery reconciliation failed: {error}"), + } + let state = rest::ApiState::new(config.clone(), config.tickets_path()); rest::serve(state, port).await?; diff --git a/src/mcp/tools.rs b/src/mcp/tools.rs index ddb21acc..7ec294b9 100644 --- a/src/mcp/tools.rs +++ b/src/mcp/tools.rs @@ -265,6 +265,11 @@ fn require_write_tools(state: &ApiState, scopes: &[Scope]) -> Result<(), String> } Ok(()) } +/// Render an `ApiError` as a stable `code: message` string. +fn mcp_error(error: crate::rest::error::ApiError) -> String { + let (_, code, message) = error.parts(); + format!("{code}: {message}") +} /// Execute an MCP tool by name with the given arguments pub async fn execute_tool( @@ -388,7 +393,7 @@ pub async fn execute_tool( .await; match result { Ok(resp) => serde_json::to_value(&*resp).map_err(|e| e.to_string()), - Err(e) => Err(format!("{e:?}")), + Err(e) => Err(mcp_error(e)), } } "operator_pause_queue" => { @@ -396,7 +401,7 @@ pub async fn execute_tool( let result = routes::queue::pause(State(state.clone())).await; match result { Ok(resp) => serde_json::to_value(&*resp).map_err(|e| e.to_string()), - Err(e) => Err(format!("{e:?}")), + Err(e) => Err(mcp_error(e)), } } "operator_resume_queue" => { @@ -404,7 +409,7 @@ pub async fn execute_tool( let result = routes::queue::resume(State(state.clone())).await; match result { Ok(resp) => serde_json::to_value(&*resp).map_err(|e| e.to_string()), - Err(e) => Err(format!("{e:?}")), + Err(e) => Err(mcp_error(e)), } } "operator_sync_kanban" => { @@ -412,7 +417,7 @@ pub async fn execute_tool( let result = routes::queue::sync(State(state.clone())).await; match result { Ok(resp) => serde_json::to_value(&*resp).map_err(|e| e.to_string()), - Err(e) => Err(format!("{e:?}")), + Err(e) => Err(mcp_error(e)), } } "operator_approve_agent" => { @@ -425,7 +430,7 @@ pub async fn execute_tool( routes::agents::approve_review(State(state.clone()), Path(id.to_string())).await; match result { Ok(resp) => serde_json::to_value(&*resp).map_err(|e| e.to_string()), - Err(e) => Err(format!("{e:?}")), + Err(e) => Err(mcp_error(e)), } } "operator_reject_agent" => { @@ -449,7 +454,7 @@ pub async fn execute_tool( .await; match result { Ok(resp) => serde_json::to_value(&*resp).map_err(|e| e.to_string()), - Err(e) => Err(format!("{e:?}")), + Err(e) => Err(mcp_error(e)), } } _ => Err(format!("Unknown tool: {name}")), @@ -459,6 +464,26 @@ pub async fn execute_tool( #[cfg(test)] mod tests { use super::*; + + /// A drain rejection must be machine-readable and carry the same code the + /// HTTP surface uses, not a debug-formatted Rust variant. + #[test] + fn test_mcp_error_uses_the_shared_stable_code_table() { + use crate::rest::error::ApiError; + + assert_eq!( + mcp_error(ApiError::Unavailable("draining".to_string())), + "unavailable: draining" + ); + assert_eq!( + mcp_error(ApiError::Conflict("already running".to_string())), + "conflict: already running" + ); + assert!( + !mcp_error(ApiError::NotFound("x".to_string())).contains("NotFound("), + "the MCP error must not leak Rust debug syntax" + ); + } use crate::config::Config; use std::path::PathBuf; diff --git a/src/rest/error.rs b/src/rest/error.rs index 8ce35110..66b9803f 100644 --- a/src/rest/error.rs +++ b/src/rest/error.rs @@ -51,9 +51,10 @@ pub struct ErrorResponse { pub message: String, } -impl IntoResponse for ApiError { - fn into_response(self) -> Response { - let (status, error, message) = match self { +impl ApiError { + /// Status, stable machine-readable code, and human message. + pub fn parts(self) -> (StatusCode, &'static str, String) { + match self { ApiError::NotFound(msg) => (StatusCode::NOT_FOUND, "not_found", msg), ApiError::ValidationError(msg) => (StatusCode::BAD_REQUEST, "validation_error", msg), ApiError::Conflict(msg) => (StatusCode::CONFLICT, "conflict", msg), @@ -66,7 +67,13 @@ impl IntoResponse for ApiError { ApiError::Unauthorized(msg) => (StatusCode::UNAUTHORIZED, "unauthorized", msg), ApiError::Forbidden(msg) => (StatusCode::FORBIDDEN, "forbidden", msg), ApiError::CsrfFailed(msg) => (StatusCode::FORBIDDEN, "csrf_failed", msg), - }; + } + } +} + +impl IntoResponse for ApiError { + fn into_response(self) -> Response { + let (status, error, message) = self.parts(); let body = Json(ErrorResponse { error: error.to_string(), diff --git a/src/rest/mod.rs b/src/rest/mod.rs index 2164510f..9f15a35e 100644 --- a/src/rest/mod.rs +++ b/src/rest/mod.rs @@ -446,7 +446,7 @@ async fn shutdown_signal(state: state::ApiState) { let config = state.config(); let drain_deadline = - Instant::now() + Duration::from_secs(config.rest_api.shutdown_drain_seconds); + Instant::now() + Duration::from_secs(u64::from(config.rest_api.shutdown_drain_seconds)); tracing::info!( drain_seconds = config.rest_api.shutdown_drain_seconds, "Shutdown drain started" @@ -475,7 +475,7 @@ async fn shutdown_signal(state: state::ApiState) { state.start_stopping(); state.mcp_sessions.lock().await.clear(); let cleanup_deadline = - Instant::now() + Duration::from_secs(config.rest_api.shutdown_cleanup_seconds); + Instant::now() + Duration::from_secs(u64::from(config.rest_api.shutdown_cleanup_seconds)); while state.in_flight_operations() != 0 && Instant::now() < cleanup_deadline { tokio::time::sleep(Duration::from_millis(50)).await; } @@ -504,7 +504,6 @@ fn interrupt_agents_for_shutdown(config: &crate::config::Config) -> Result<()> { const INTERRUPTION_MESSAGE: &str = "Interrupted by Operator shutdown; retry explicitly"; - let mut app_state = crate::state::State::load(config)?; let launcher = match crate::agents::Launcher::new(config) { Ok(launcher) => Some(launcher), Err(error) => { @@ -512,48 +511,52 @@ fn interrupt_agents_for_shutdown(config: &crate::config::Config) -> Result<()> { None } }; - for agent in &mut app_state.agents { - if !matches!( - agent.status.as_str(), - "running" | "awaiting_input" | "completing" - ) { - continue; - } - let configured_remote = agent - .target_name - .as_deref() - .and_then(|name| config.targets.iter().find(|target| target.name == name)) - .is_some_and(|target| { - matches!( - &target.kind, - crate::config::TargetKind::Coder(_) | crate::config::TargetKind::Ssh(_) - ) - }); - let remote = agent.remote_host.is_some() - || configured_remote - || agent - .launch_mode + + // Reread under the write lock: a completion callback may have landed between the drain loop's last poll and here + crate::state::State::mutate(config, |app_state| { + for agent in &mut app_state.agents { + if !matches!( + agent.status.as_str(), + "running" | "awaiting_input" | "completing" + ) { + continue; + } + let configured_remote = agent + .target_name .as_deref() - .is_some_and(|mode| mode.starts_with("coder") || mode.starts_with("ssh")); - if remote { - agent.shutdown_recovery = Some(ShutdownRecovery::RemoteAwaitingReconciliation); - agent.last_message = Some("Remote work preserved during Operator shutdown".to_string()); - continue; - } + .and_then(|name| config.targets.iter().find(|target| target.name == name)) + .is_some_and(|target| { + matches!( + &target.kind, + crate::config::TargetKind::Coder(_) | crate::config::TargetKind::Ssh(_) + ) + }); + let remote = agent.remote_host.is_some() + || configured_remote + || agent + .launch_mode + .as_deref() + .is_some_and(|mode| mode.starts_with("coder") || mode.starts_with("ssh")); + if remote { + agent.shutdown_recovery = Some(ShutdownRecovery::RemoteAwaitingReconciliation); + agent.last_message = + Some("Remote work preserved during Operator shutdown".to_string()); + continue; + } - if agent.session_name.is_some() { - if let Some(launcher) = &launcher { - if let Err(error) = launcher.kill_local_agent_session(agent) { - tracing::warn!(%error, agent_id = %agent.id, "Failed to stop local session during shutdown"); + if agent.session_name.is_some() { + if let Some(launcher) = &launcher { + if let Err(error) = launcher.kill_local_agent_session(agent) { + tracing::warn!(%error, agent_id = %agent.id, "Failed to stop local session during shutdown"); + } } } + agent.status = "failed".to_string(); + agent.last_message = Some(INTERRUPTION_MESSAGE.to_string()); + agent.shutdown_recovery = Some(ShutdownRecovery::InterruptedLocal); + agent.last_activity = chrono::Utc::now(); } - agent.status = "failed".to_string(); - agent.last_message = Some(INTERRUPTION_MESSAGE.to_string()); - agent.shutdown_recovery = Some(ShutdownRecovery::InterruptedLocal); - agent.last_activity = chrono::Utc::now(); - } - app_state.save() + }) } #[cfg(test)] @@ -570,6 +573,119 @@ mod tests { // Router builds without panicking } + fn shutdown_config(temp: &tempfile::TempDir) -> Config { + let mut config = Config::default(); + config.paths.state = temp.path().join("state").display().to_string(); + config.paths.tickets = temp.path().join("tickets").display().to_string(); + config.paths.projects = temp.path().join("projects").display().to_string(); + config + } + + fn seed_agent(config: &Config, ticket: &str, launch_mode: Option<&str>) -> String { + crate::state::State::mutate(config, |state| { + state + .add_agent_with_options( + ticket.to_string(), + "FEAT".to_string(), + "project".to_string(), + false, + None, + launch_mode.map(str::to_string), + ) + .unwrap() + }) + .unwrap() + } + + /// Shutdown bookkeeping must not resurrect an agent that reported completion + /// while the drain was running. Both writes go through `State::mutate`, so + /// the later one rereads the earlier one's result. + #[test] + fn shutdown_does_not_overwrite_a_completion_recorded_during_the_drain() { + let temp = tempfile::TempDir::new().unwrap(); + let config = shutdown_config(&temp); + let finished = seed_agent(&config, "DONE-1", None); + let still_running = seed_agent(&config, "BUSY-1", None); + + // The callback lands mid-drain, before cleanup runs. + crate::state::State::mutate(&config, |state| { + let agent = state + .agents + .iter_mut() + .find(|agent| agent.id == finished) + .unwrap(); + agent.status = "completed".to_string(); + }) + .unwrap(); + + interrupt_agents_for_shutdown(&config).unwrap(); + + let state = crate::state::State::load(&config).unwrap(); + let done = state.agents.iter().find(|a| a.id == finished).unwrap(); + assert_eq!( + done.status, "completed", + "a completion recorded during the drain must survive shutdown bookkeeping" + ); + assert_eq!( + done.shutdown_recovery, None, + "finished work needs no recovery marker" + ); + + let busy = state.agents.iter().find(|a| a.id == still_running).unwrap(); + assert_eq!(busy.status, "failed"); + assert_eq!( + busy.shutdown_recovery, + Some(crate::state::ShutdownRecovery::InterruptedLocal) + ); + } + + /// Running shutdown bookkeeping twice (a second signal, or a retry) must be + /// idempotent rather than compounding. + #[test] + fn shutdown_bookkeeping_is_idempotent() { + let temp = tempfile::TempDir::new().unwrap(); + let config = shutdown_config(&temp); + let id = seed_agent(&config, "LOCAL-2", None); + + interrupt_agents_for_shutdown(&config).unwrap(); + let first = crate::state::State::load(&config).unwrap(); + interrupt_agents_for_shutdown(&config).unwrap(); + let second = crate::state::State::load(&config).unwrap(); + + let before = first.agents.iter().find(|a| a.id == id).unwrap(); + let after = second.agents.iter().find(|a| a.id == id).unwrap(); + assert_eq!(before.status, after.status); + assert_eq!(before.shutdown_recovery, after.shutdown_recovery); + assert_eq!(second.agents.len(), 1, "no records may be duplicated"); + } + + /// Remote work is left alone on the way down and picked up by startup + /// reconciliation, which must not resolve it either. + #[test] + fn remote_work_survives_shutdown_and_restart_as_unresolved() { + let temp = tempfile::TempDir::new().unwrap(); + let config = shutdown_config(&temp); + let id = seed_agent(&config, "REMOTE-2", Some("coder")); + + interrupt_agents_for_shutdown(&config).unwrap(); + crate::startup::recovery::reconcile(&config).unwrap(); + + let agent = crate::state::State::load(&config) + .unwrap() + .agents + .into_iter() + .find(|a| a.id == id) + .unwrap(); + assert_eq!( + agent.status, "running", + "a surviving remote workspace must not be failed or completed by us" + ); + assert_eq!( + agent.shutdown_recovery, + Some(crate::state::ShutdownRecovery::RemoteAwaitingReconciliation) + ); + } + #[test] fn shutdown_marks_local_work_interrupted_and_preserves_remote_work() { let temp = tempfile::TempDir::new().unwrap(); diff --git a/src/rest/routes/launch.rs b/src/rest/routes/launch.rs index f405989d..3d826c2d 100644 --- a/src/rest/routes/launch.rs +++ b/src/rest/routes/launch.rs @@ -161,7 +161,21 @@ pub async fn launch_ticket( Path(ticket_id): Path, Json(request): Json, ) -> Result, ApiError> { - let _launch_permit = state.begin_launch()?; + let permit = state.begin_launch()?; + tokio::spawn(async move { + let _launch_permit = permit; + launch_admitted_ticket(state, ticket_id, request).await + }) + .await + .map_err(|error| ApiError::InternalError(format!("Launch task failed: {error}")))? +} + +/// The launch itself, past admission control. +async fn launch_admitted_ticket( + state: ApiState, + ticket_id: String, + request: LaunchTicketRequest, +) -> Result, ApiError> { let recovery_state = crate::state::State::load(&state.config()) .map_err(|error| ApiError::InternalError(error.to_string()))?; if recovery_state.agents.iter().any(|agent| { @@ -258,24 +272,21 @@ pub async fn launch_ticket( } }; - let mut recovery_state = crate::state::State::load(&state.config()) - .map_err(|error| ApiError::InternalError(error.to_string()))?; - let mut recovery_cleared = false; - for agent in recovery_state - .agents - .iter_mut() - .filter(|agent| agent.ticket_id == ticket_id) - { - if agent.shutdown_recovery == Some(crate::state::ShutdownRecovery::InterruptedLocal) { - agent.shutdown_recovery = None; - recovery_cleared = true; + // An explicit relaunch is the retry this ticket's interrupted local work was + // waiting for, so clear the marker. Under the write lock, because the launch + // above has already registered its own agent record. + crate::state::State::mutate(&state.config(), |app_state| { + for agent in app_state + .agents + .iter_mut() + .filter(|agent| agent.ticket_id == ticket_id) + { + if agent.shutdown_recovery == Some(crate::state::ShutdownRecovery::InterruptedLocal) { + agent.shutdown_recovery = None; + } } - } - if recovery_cleared { - recovery_state - .save() - .map_err(|error| ApiError::InternalError(error.to_string()))?; - } + }) + .map_err(|error| ApiError::InternalError(error.to_string()))?; Ok(Json(response)) } @@ -1702,6 +1713,98 @@ mod tests { ); } + /// During the drain window a completion must still be recorded - the agent + /// did the work - but Operator must not hand back anything that would start + /// the next step in a process that is going away. + #[tokio::test] + async fn test_complete_step_records_work_but_defers_progression_while_draining() { + let fixture = make_chain_fixture(); + let ticket = write_sync_ticket(&fixture.state, "SYNC-9010", "scan"); + add_chain_agent(&fixture.state, &ticket, "sonnet", "session-scan"); + + assert!(fixture.state.start_draining()); + + let response = complete_step( + State(fixture.state.clone()), + Path((ticket.id.clone(), "scan".to_string())), + Authenticated(crate::auth::scope::Principal::local("admin")), + Json(make_chain_complete_request("session-scan")), + ) + .await + .expect("a completion callback must still be accepted while draining"); + + assert!( + !response.0.auto_proceed, + "draining must not auto-advance the chain" + ); + assert!( + response.0.next_command.is_none(), + "no executable next-step instruction may be returned while draining" + ); + assert!( + response.0.next_step.is_some(), + "the caller is still told what comes next, just not told to run it" + ); + } + + /// Once stopping, even callbacks are refused; the drain window is over. + #[tokio::test] + async fn test_complete_step_is_refused_once_stopping() { + let fixture = make_chain_fixture(); + let ticket = write_sync_ticket(&fixture.state, "SYNC-9011", "scan"); + add_chain_agent(&fixture.state, &ticket, "sonnet", "session-scan"); + + fixture.state.start_draining(); + fixture.state.start_stopping(); + + let result = complete_step( + State(fixture.state.clone()), + Path((ticket.id.clone(), "scan".to_string())), + Authenticated(crate::auth::scope::Principal::local("admin")), + Json(make_chain_complete_request("session-scan")), + ) + .await; + + assert!(matches!(result, Err(ApiError::Unavailable(_)))); + } + + /// Launch admission is closed for every caller that reaches this handler, + /// REST and MCP alike, and the refusal is a 503-shaped error. + #[tokio::test] + async fn test_launch_ticket_is_refused_while_draining() { + let fixture = make_chain_fixture(); + let ticket = write_sync_ticket(&fixture.state, "SYNC-9012", "scan"); + + fixture.state.start_draining(); + + let result = launch_ticket( + State(fixture.state.clone()), + Path(ticket.id.clone()), + Json(LaunchTicketRequest { + target: None, + delegator: None, + provider: None, + model: None, + model_server: None, + yolo_mode: false, + wrapper: None, + retry_reason: None, + resume_session_id: None, + }), + ) + .await; + + match result { + Err(ApiError::Unavailable(message)) => { + assert!( + message.contains("draining"), + "unexpected message: {message}" + ); + } + other => panic!("expected an Unavailable rejection, got {other:?}"), + } + } + #[tokio::test] async fn test_complete_step_duplicate_post_does_not_double_advance() { let fixture = make_chain_fixture(); diff --git a/src/rest/state.rs b/src/rest/state.rs index 3ae2e21c..b480beb9 100644 --- a/src/rest/state.rs +++ b/src/rest/state.rs @@ -126,7 +126,6 @@ impl ApiState { /// Build with a caller-supplied auth context, so tests and the TUI can share /// one already-open store instead of racing to open the same database file. pub fn with_auth(config: Config, tickets_path: PathBuf, auth: Arc) -> Self { - reconcile_shutdown_recovery(&config); // Shared loader; keeps the API's issue-type resolution identical to the CLI/TUI `workflow export` produces the same output on every surface. let registry = load_registry(&tickets_path); @@ -265,39 +264,6 @@ impl ApiState { } } -fn reconcile_shutdown_recovery(config: &Config) { - let Ok(mut app_state) = crate::state::State::load(config) else { - return; - }; - let mut changed = false; - for agent in &mut app_state.agents { - match agent.shutdown_recovery { - Some(crate::state::ShutdownRecovery::RemoteAwaitingReconciliation) => { - if matches!( - agent.status.as_str(), - "running" | "awaiting_input" | "completing" - ) { - agent.last_message = Some( - "Remote work is awaiting reconciliation after Operator restart".to_string(), - ); - } else { - agent.shutdown_recovery = None; - } - changed = true; - } - Some(crate::state::ShutdownRecovery::InterruptedLocal) => { - agent.status = "failed".to_string(); - } - None => {} - } - } - if changed { - if let Err(error) = app_state.save() { - tracing::error!(%error, "Failed to persist shutdown recovery state"); - } - } -} - #[cfg(test)] mod tests { use super::*; @@ -368,6 +334,118 @@ mod tests { ); } + fn lifecycle_state(temp: &tempfile::TempDir) -> ApiState { + let mut config = Config::default(); + config.paths.state = temp.path().join("state").display().to_string(); + config.paths.tickets = temp.path().join("tickets").display().to_string(); + ApiState::new(config, temp.path().join("tickets")) + } + + /// A launch admitted moments before the drain begins must stay counted + /// until it finishes; the drain waits for it rather than racing past. + #[test] + fn admitted_launch_keeps_the_drain_waiting_until_it_finishes() { + let temp = tempfile::TempDir::new().unwrap(); + let state = lifecycle_state(&temp); + + let permit = state.begin_launch().unwrap(); + assert!(state.start_draining()); + assert_eq!( + state.in_flight_operations(), + 1, + "draining must not discard an already-admitted launch" + ); + + drop(permit); + assert_eq!(state.in_flight_operations(), 0); + } + + /// Repeated signals must not reopen admission or restart the deadline; only + /// the first transition out of Running wins. + #[test] + fn repeated_drain_signals_do_not_reopen_admission() { + let temp = tempfile::TempDir::new().unwrap(); + let state = lifecycle_state(&temp); + + assert!(state.start_draining(), "first signal starts the drain"); + assert!( + !state.start_draining(), + "a second signal must not restart the drain" + ); + assert!(!state.start_draining()); + assert!(state.is_draining()); + assert!(state.begin_launch().is_err()); + } + + /// Callbacks stay open through the drain window so work in flight can report + /// completion; only the stopping phase closes them. + #[test] + fn callbacks_stay_open_through_draining_and_close_on_stopping() { + let temp = tempfile::TempDir::new().unwrap(); + let state = lifecycle_state(&temp); + + state.start_draining(); + let callback = state + .begin_callback() + .expect("a completion callback must still be accepted while draining"); + assert_eq!(state.in_flight_operations(), 1); + drop(callback); + + state.start_stopping(); + assert!(matches!( + state.begin_callback(), + Err(ApiError::Unavailable(_)) + )); + } + + /// Permits are counted, not boolean: concurrent launches must each be + /// tracked, and the count must return to zero exactly. + #[test] + fn concurrent_permits_are_counted_independently() { + let temp = tempfile::TempDir::new().unwrap(); + let state = lifecycle_state(&temp); + + let permits: Vec<_> = (0..5).map(|_| state.begin_launch().unwrap()).collect(); + assert_eq!(state.in_flight_operations(), 5); + + state.start_draining(); + drop(permits); + assert_eq!(state.in_flight_operations(), 0); + } + + /// Admission is checked again after the counter is incremented, so a drain + /// landing between the two cannot leak an uncounted permit. + #[test] + fn launch_admission_under_concurrent_drain_never_leaks_a_permit() { + let temp = tempfile::TempDir::new().unwrap(); + let state = lifecycle_state(&temp); + + std::thread::scope(|scope| { + let drainer = { + let state = state.clone(); + scope.spawn(move || state.start_draining()) + }; + let launchers: Vec<_> = (0..16) + .map(|_| { + let state = state.clone(); + scope.spawn(move || state.begin_launch().ok()) + }) + .collect(); + + drainer.join().unwrap(); + for launcher in launchers { + drop(launcher.join().unwrap()); + } + }); + + assert_eq!( + state.in_flight_operations(), + 0, + "every admitted permit must be released and every rejection must not count" + ); + assert!(state.begin_launch().is_err()); + } + #[test] fn draining_closes_launch_admission_without_losing_in_flight_tracking() { let temp = tempfile::TempDir::new().unwrap(); diff --git a/src/startup/mod.rs b/src/startup/mod.rs index b07874e4..e6a4fd29 100644 --- a/src/startup/mod.rs +++ b/src/startup/mod.rs @@ -21,6 +21,7 @@ //! init_default_templates(&templates_path)?; //! ``` +pub mod recovery; pub mod steps; pub mod templates; diff --git a/src/startup/recovery.rs b/src/startup/recovery.rs new file mode 100644 index 00000000..ec01e214 --- /dev/null +++ b/src/startup/recovery.rs @@ -0,0 +1,195 @@ +//! Shutdown recovery reconciliation, run once before the API accepts launches. +//! +//! Operator marks agents on the way down (`ShutdownRecovery`); this decides what +//! those marks mean on the way back up. It deliberately does not probe remote +//! hosts: a Coder workspace or SSH session can outlive Operator while its +//! callback path is down, so unreachable is reported as unresolved rather than +//! guessed either way. + +use crate::config::Config; +use crate::state::{ShutdownRecovery, State}; + +const LOCAL_MESSAGE: &str = "Interrupted by Operator shutdown; retry explicitly"; +const REMOTE_MESSAGE: &str = "Remote work is awaiting reconciliation after Operator restart"; + +/// Statuses that mean an agent still holds work. +fn is_active(status: &str) -> bool { + matches!(status, "running" | "awaiting_input" | "completing") +} + +/// Reconcile shutdown markers. Returns how many agent records changed. +pub fn reconcile(config: &Config) -> anyhow::Result { + State::mutate(config, |state| { + let mut changed = 0; + for agent in &mut state.agents { + match agent.shutdown_recovery { + // Remote work was left running on purpose. Keep it flagged so a + // relaunch of the same ticket is refused until an operator says + // whether it survived. + Some(ShutdownRecovery::RemoteAwaitingReconciliation) => { + if is_active(&agent.status) { + agent.last_message = Some(REMOTE_MESSAGE.to_string()); + } else { + // It reported a terminal status after the mark was + // written, so there is nothing left to reconcile. + agent.shutdown_recovery = None; + } + changed += 1; + } + // The local session is gone with the process. Leave it failed + // and retryable; never auto-relaunch. + Some(ShutdownRecovery::InterruptedLocal) => { + if is_active(&agent.status) { + agent.status = "failed".to_string(); + agent.last_message = Some(LOCAL_MESSAGE.to_string()); + } + changed += 1; + } + None => {} + } + } + changed + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn config(temp: &tempfile::TempDir) -> Config { + let mut config = Config::default(); + config.paths.state = temp.path().join("state").display().to_string(); + config.paths.tickets = temp.path().join("tickets").display().to_string(); + config + } + + fn seed(config: &Config, ticket: &str, launch_mode: Option<&str>) -> String { + State::mutate(config, |state| { + state + .add_agent_with_options( + ticket.to_string(), + "FEAT".to_string(), + "project".to_string(), + false, + None, + launch_mode.map(str::to_string), + ) + .unwrap() + }) + .unwrap() + } + + fn mark(config: &Config, id: &str, recovery: ShutdownRecovery, status: &str) { + State::mutate(config, |state| { + let agent = state.agents.iter_mut().find(|a| a.id == id).unwrap(); + agent.shutdown_recovery = Some(recovery); + agent.status = status.to_string(); + }) + .unwrap(); + } + + fn agent(config: &Config, id: &str) -> crate::state::AgentState { + State::load(config) + .unwrap() + .agents + .into_iter() + .find(|a| a.id == id) + .unwrap() + } + + /// Regression: the local arm mutated the record without flagging the write, + /// so the status change was silently discarded. + #[test] + fn test_interrupted_local_work_persists_as_failed_and_retryable() { + let temp = tempfile::TempDir::new().unwrap(); + let config = config(&temp); + let id = seed(&config, "FEAT-1", None); + mark(&config, &id, ShutdownRecovery::InterruptedLocal, "running"); + + assert_eq!(reconcile(&config).unwrap(), 1); + + let agent = agent(&config, &id); + assert_eq!(agent.status, "failed"); + assert_eq!(agent.last_message.as_deref(), Some(LOCAL_MESSAGE)); + assert_eq!( + agent.shutdown_recovery, + Some(ShutdownRecovery::InterruptedLocal), + "the marker stays so an explicit retry can clear it" + ); + } + + #[test] + fn test_remote_work_stays_unresolved_and_is_never_marked_completed() { + let temp = tempfile::TempDir::new().unwrap(); + let config = config(&temp); + let id = seed(&config, "FEAT-2", Some("coder")); + mark( + &config, + &id, + ShutdownRecovery::RemoteAwaitingReconciliation, + "running", + ); + + reconcile(&config).unwrap(); + + let agent = agent(&config, &id); + assert_eq!( + agent.status, "running", + "surviving remote work must not be failed or completed on our say-so" + ); + assert_eq!(agent.last_message.as_deref(), Some(REMOTE_MESSAGE)); + assert_eq!( + agent.shutdown_recovery, + Some(ShutdownRecovery::RemoteAwaitingReconciliation) + ); + } + + /// A completion recorded during downtime is authoritative; clear the mark. + #[test] + fn test_remote_work_that_completed_during_downtime_clears_its_marker() { + let temp = tempfile::TempDir::new().unwrap(); + let config = config(&temp); + let id = seed(&config, "FEAT-3", Some("coder")); + mark( + &config, + &id, + ShutdownRecovery::RemoteAwaitingReconciliation, + "completed", + ); + + reconcile(&config).unwrap(); + + let agent = agent(&config, &id); + assert_eq!(agent.status, "completed"); + assert_eq!(agent.shutdown_recovery, None); + } + + #[test] + fn test_unmarked_agents_are_left_alone() { + let temp = tempfile::TempDir::new().unwrap(); + let config = config(&temp); + let id = seed(&config, "FEAT-4", None); + + assert_eq!(reconcile(&config).unwrap(), 0); + assert_eq!(agent(&config, &id).shutdown_recovery, None); + } + + /// State written before the marker existed must still load and reconcile. + #[test] + fn test_state_without_the_marker_field_reconciles_cleanly() { + let temp = tempfile::TempDir::new().unwrap(); + let config = config(&temp); + let id = seed(&config, "FEAT-5", None); + + let path = std::path::PathBuf::from(&config.paths.state).join("state.json"); + let raw = std::fs::read_to_string(&path).unwrap(); + assert!( + !raw.contains("shutdown_recovery"), + "an unmarked agent must not serialize the field at all" + ); + std::fs::write(&path, raw).unwrap(); + + assert_eq!(reconcile(&config).unwrap(), 0); + assert_eq!(agent(&config, &id).shutdown_recovery, None); + } +} diff --git a/src/state.rs b/src/state.rs index fd16be4b..7d245e81 100644 --- a/src/state.rs +++ b/src/state.rs @@ -13,6 +13,37 @@ use uuid::Uuid; use crate::config::Config; use crate::types::llm_stats::ProjectLlmStats; +const STATE_FILE: &str = "state.json"; +const STATE_TEMP_FILE: &str = "state.json.tmp"; + +/// Serializes every `state.json` read-modify-write in this process. One +/// Operator process owns one state directory, so a single lock is enough. +struct WriteLock(Option>); + +thread_local! { + static LOCK_HELD: std::cell::Cell = const { std::cell::Cell::new(false) }; +} + +impl Drop for WriteLock { + fn drop(&mut self) { + if self.0.is_some() { + LOCK_HELD.with(|held| held.set(false)); + } + } +} + +fn write_lock() -> WriteLock { + static LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + if LOCK_HELD.with(std::cell::Cell::get) { + return WriteLock(None); + } + let guard = LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + LOCK_HELD.with(|held| held.set(true)); + WriteLock(Some(guard)) +} + #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, TS)] #[ts(export)] pub struct State { @@ -105,7 +136,6 @@ pub struct AgentState { #[serde(default)] pub llm_model: Option, /// Launch mode: `default|yolo|docker[-yolo]|coder[-yolo]|ssh[-yolo]` - /// (derived from the resolved execution target; parse with `agents::parse_launch_mode`, never substring-match) #[serde(default)] pub launch_mode: Option, /// Review state for `awaiting_input` agents @@ -227,10 +257,15 @@ pub enum MultiAgentPhase { impl State { pub fn load(config: &Config) -> Result { + let _guard = write_lock(); + Self::load_unlocked(config) + } + + fn load_unlocked(config: &Config) -> Result { let state_path = config.state_path(); fs::create_dir_all(&state_path).context("Failed to create state directory")?; - let state_file = state_path.join("state.json"); + let state_file = state_path.join(STATE_FILE); if state_file.exists() { let contents = fs::read_to_string(&state_file).context("Failed to read state file")?; @@ -252,9 +287,31 @@ impl State { } pub fn save(&self) -> Result<()> { - let state_file = self.state_path.join("state.json"); + let _guard = write_lock(); + self.write_unlocked() + } + + /// Read-modify-write under the process write lock, rereading from disk + /// first so a concurrent writer's committed changes are not clobbered. + /// Every mutation that races with another writer belongs here, not in a + /// `load` / mutate / `save` triple. + pub fn mutate(config: &Config, apply: impl FnOnce(&mut Self) -> T) -> Result { + let _guard = write_lock(); + let mut state = Self::load_unlocked(config)?; + let outcome = apply(&mut state); + state.write_unlocked()?; + Ok(outcome) + } + + /// Write via a sibling temp file and rename + fn write_unlocked(&self) -> Result<()> { + let state_file = self.state_path.join(STATE_FILE); + let temp_file = self.state_path.join(STATE_TEMP_FILE); let contents = serde_json::to_string_pretty(self)?; - fs::write(state_file, contents)?; + fs::write(&temp_file, contents) + .with_context(|| format!("Failed to write {}", temp_file.display()))?; + fs::rename(&temp_file, &state_file) + .with_context(|| format!("Failed to commit {}", state_file.display()))?; Ok(()) } @@ -1131,6 +1188,133 @@ mod tests { config } + // โ”€โ”€โ”€ Persistence Concurrency Tests โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€ + + /// The defect `mutate` exists to prevent: a writer holding a handle loaded + /// before someone else's commit writes its whole snapshot back and silently + /// discards that commit. Shutdown bookkeeping racing a completion write hit + /// exactly this, so contrast both paths against the same race. + #[test] + fn test_mutate_rereads_so_a_concurrent_write_is_not_clobbered() { + let temp_dir = TempDir::new().unwrap(); + let config = test_config(&temp_dir); + + let mut seed = State::load(&config).unwrap(); + let id = seed + .add_agent( + "FEAT-1".to_string(), + "FEAT".to_string(), + "p".to_string(), + false, + ) + .unwrap(); + + // A handle taken before the completion below - the shutdown coordinator's + // view of the world when a callback lands mid-drain. + let mut stale = State::load(&config).unwrap(); + + let complete = |config: &Config| { + State::mutate(config, |state| { + state.agents[0].status = "completed".to_string(); + }) + .unwrap(); + }; + + // Old path: whole-snapshot save from the stale handle loses the completion. + complete(&config); + stale.agents[0].last_message = Some("shutdown bookkeeping".to_string()); + stale.save().unwrap(); + assert_eq!( + State::load(&config).unwrap().agents[0].status, + "running", + "precondition: a stale whole-snapshot save is what loses the completion" + ); + + // mutate rereads under the lock, so the same bookkeeping keeps it. + complete(&config); + State::mutate(&config, |state| { + state.agents[0].last_message = Some("shutdown bookkeeping".to_string()); + }) + .unwrap(); + + let reloaded = State::load(&config).unwrap(); + let agent = reloaded.agents.iter().find(|a| a.id == id).unwrap(); + assert_eq!( + agent.status, "completed", + "mutate must reread, not overwrite a committed completion" + ); + assert_eq!(agent.last_message.as_deref(), Some("shutdown bookkeeping")); + } + + /// Most `State` methods save as they go, so `mutate` must tolerate a closure + /// that saves rather than deadlocking on its own lock. + #[test] + fn test_mutate_tolerates_a_closure_that_saves() { + let temp_dir = TempDir::new().unwrap(); + let config = test_config(&temp_dir); + + State::mutate(&config, |state| { + state.set_paused(true).unwrap(); + }) + .unwrap(); + + assert!(State::load(&config).unwrap().paused); + } + + /// The temp file must not be left behind. + #[test] + fn test_save_commits_atomically_and_leaves_no_temp_file() { + let temp_dir = TempDir::new().unwrap(); + let config = test_config(&temp_dir); + + let mut state = State::load(&config).unwrap(); + state + .add_agent( + "FEAT-1".to_string(), + "FEAT".to_string(), + "p".to_string(), + false, + ) + .unwrap(); + + assert!(temp_dir.path().join(STATE_FILE).exists()); + assert!( + !temp_dir.path().join(STATE_TEMP_FILE).exists(), + "the staging file must be renamed away, not left in the state directory" + ); + assert!(State::load(&config).is_ok(), "committed state must parse"); + } + + /// Concurrent writers from several threads must all land; none may be lost. + #[test] + fn test_concurrent_mutations_all_land() { + let temp_dir = TempDir::new().unwrap(); + let config = test_config(&temp_dir); + State::load(&config).unwrap().save().unwrap(); + + std::thread::scope(|scope| { + for index in 0..8 { + let config = &config; + scope.spawn(move || { + State::mutate(config, |state| { + state + .project_collection_prefs + .insert(format!("project-{index}"), "core".to_string()); + }) + .unwrap(); + }); + } + }); + + let reloaded = State::load(&config).unwrap(); + assert_eq!( + reloaded.project_collection_prefs.len(), + 8, + "every concurrent mutation must survive; got {:?}", + reloaded.project_collection_prefs + ); + } + // โ”€โ”€โ”€ Load/Save Tests โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€ #[test] diff --git a/tests/distribution_bundling.rs b/tests/distribution_bundling.rs index 7820145c..eed638f6 100644 --- a/tests/distribution_bundling.rs +++ b/tests/distribution_bundling.rs @@ -110,3 +110,84 @@ fn test_docker_ci_job_stages_opr8r_artifacts() { "the docker job must stage opr8r under the Dockerfile's TARGETARCH naming convention" ); } + +/// Slice one top-level job block out of build.yaml: from its ` :` +/// header up to the next two-space-indented key. +fn job_block<'a>(workflow: &'a str, name: &str) -> &'a str { + let header = format!("\n {name}:\n"); + let start = workflow + .find(&header) + .unwrap_or_else(|| panic!("build.yaml must have a top-level `{name}:` job")) + + header.len(); + let body = &workflow[start..]; + let end = body + .match_indices("\n ") + .find(|(i, _)| { + let line = &body[i + 1..]; + !line.starts_with(" ") && line.lines().next().is_some_and(|l| l.ends_with(':')) + }) + .map_or(body.len(), |(i, _)| i); + &body[..end] +} + +/// The whole point of splitting build/scan from publish is that the bytes that +/// were scanned are the bytes that get pushed. A `build-push-action` in the +/// publish job would rebuild them and defeat the blocking scan. +#[test] +fn test_docker_publish_job_never_rebuilds_the_scanned_image() { + let content = read(&repo_root().join(".github/workflows/build.yaml")); + let publish = job_block(&content, "docker-publish"); + + assert!( + !publish.contains("docker/build-push-action"), + "docker-publish must load the scanned image archives, not rebuild them; \ + a rebuild would publish bytes that Trivy never saw" + ); + assert!( + publish.contains("docker load -i"), + "docker-publish must load the exact archives exported by the scanned build jobs" + ); + assert!( + publish.contains("pattern: operator-image-*"), + "docker-publish must download the per-architecture scanned image artifacts" + ); +} + +/// Chart publication must trail successful image publication, so a scan failure +/// cannot leave a chart pointing at an image tag that was never published. +#[test] +fn test_chart_job_depends_on_successful_image_publication() { + let content = read(&repo_root().join(".github/workflows/build.yaml")); + let chart = job_block(&content, "chart"); + + let needs = chart + .lines() + .find(|line| line.trim_start().starts_with("needs:")) + .expect("the chart job must declare `needs:`"); + assert!( + needs.contains("docker-publish"), + "the chart job must depend on docker-publish so a blocked image scan also blocks \ + chart publication; found {needs:?}" + ); + assert!( + needs.contains("release"), + "the chart job must depend on release to package from the version-bump commit; \ + found {needs:?}" + ); + assert!( + chart.contains("ref: ${{ needs.release.outputs.commit }}"), + "the chart job must check out the release commit so chart version, appVersion \ + and image version match the published release" + ); +} + +#[test] +fn test_job_block_slices_a_single_job() { + let content = read(&repo_root().join(".github/workflows/build.yaml")); + let publish = job_block(&content, "docker-publish"); + assert!(publish.contains("Push scanned platform images")); + assert!( + !publish.contains("Package and push Helm chart"), + "job_block leaked into the following job" + ); +} diff --git a/tests/helm_chart.rs b/tests/helm_chart.rs new file mode 100644 index 00000000..2f544229 --- /dev/null +++ b/tests/helm_chart.rs @@ -0,0 +1,225 @@ +//! Chart packaging and rendering guarantees that `helm lint` does not cover. +//! +//! Skips cleanly when `helm` is absent so a contributor without it can still +//! run the suite; CI always has it (`azure/setup-helm` in the lint-test job). + +use std::path::{Path, PathBuf}; +use std::process::Command; + +fn repo_root() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) +} + +fn chart_dir() -> PathBuf { + repo_root().join("charts/operator") +} + +fn helm_available() -> bool { + Command::new("helm") + .arg("version") + .output() + .is_ok_and(|out| out.status.success()) +} + +/// Run helm and return stdout, asserting it succeeded. +fn helm(args: &[&str]) -> String { + let out = Command::new("helm") + .args(args) + .output() + .expect("failed to run helm"); + assert!( + out.status.success(), + "helm {args:?} failed:\n{}", + String::from_utf8_lossy(&out.stderr) + ); + String::from_utf8_lossy(&out.stdout).into_owned() +} + +/// `helm install --dry-run` is the only way to render NOTES.txt; `helm template` +/// skips it. +fn rendered_notes(args: &[&str]) -> String { + let mut full = vec!["install", "op"]; + let dir = chart_dir(); + let dir = dir.to_str().unwrap(); + full.push(dir); + full.push("--dry-run"); + full.extend_from_slice(args); + let output = helm(&full); + output + .split_once("NOTES:") + .map(|(_, notes)| notes.trim().to_string()) + .expect("helm install --dry-run must render NOTES.txt") +} + +/// The README is the packaged install guide - `helm show readme` and the GHCR +/// listing both read it out of the archive. Packaging it is easy to lose. +#[test] +fn test_readme_is_packaged_into_the_chart_archive() { + if !helm_available() { + eprintln!("skipping: helm not on PATH"); + return; + } + let dest = tempfile::TempDir::new().unwrap(); + helm(&[ + "package", + chart_dir().to_str().unwrap(), + "--destination", + dest.path().to_str().unwrap(), + ]); + + let archive = std::fs::read_dir(dest.path()) + .unwrap() + .filter_map(Result::ok) + .map(|entry| entry.path()) + .find(|path| path.extension().is_some_and(|ext| ext == "tgz")) + .expect("helm package must produce a .tgz"); + + let listing = Command::new("tar") + .arg("-tzf") + .arg(&archive) + .output() + .expect("failed to list the chart archive"); + let listing = String::from_utf8_lossy(&listing.stdout); + + assert!( + listing.lines().any(|entry| entry == "operator/README.md"), + "operator/README.md is missing from the packaged chart; archive contains:\n{listing}" + ); +} + +/// The ingress branch of NOTES dereferences `.Values.ingress.tls.secretName`, +/// which the default render never reaches. +#[test] +fn test_notes_render_a_usable_setup_url_on_both_branches() { + if !helm_available() { + eprintln!("skipping: helm not on PATH"); + return; + } + + let ingress = rendered_notes(&[ + "--set", + "ingress.enabled=true", + "--set", + "ingress.host=op.example.com", + "--set", + "ingress.tls.secretName=op-tls", + ]); + assert!( + ingress.contains("https://op.example.com/setup"), + "TLS ingress NOTES must link the https /setup URL:\n{ingress}" + ); + + let forwarded = rendered_notes(&["--namespace", "ops"]); + assert!( + forwarded.contains("kubectl -n ops port-forward service/op-operator 7008:7008"), + "port-forward command must carry the release namespace:\n{forwarded}" + ); + assert!( + forwarded.contains("http://127.0.0.1:7008/setup"), + "NOTES must give the local /setup URL when no ingress is configured:\n{forwarded}" + ); +} + +/// `publicUrl` only changes generated links; it provisions no Ingress. Saying +/// otherwise sends operators looking for a route that does not exist. +#[test] +fn test_notes_separate_public_url_from_ingress() { + if !helm_available() { + eprintln!("skipping: helm not on PATH"); + return; + } + let notes = rendered_notes(&["--set", "publicUrl=https://op.example.com"]); + assert!( + notes.contains("publicUrl does not create an Ingress"), + "NOTES must state that publicUrl creates no Ingress:\n{notes}" + ); + assert!( + notes.contains("http://127.0.0.1:7008/setup"), + "the local /setup URL must still be shown alongside configured public access:\n{notes}" + ); +} + +/// The packaged README is not Jekyll-rendered, so its value table is hand-kept +/// and drifts silently from values.yaml. +#[test] +fn test_packaged_readme_documents_the_current_default_values() { + let values = std::fs::read_to_string(chart_dir().join("values.yaml")).unwrap(); + let readme = std::fs::read_to_string(chart_dir().join("README.md")).unwrap(); + + for key in [ + "extraVolumes", + "extraVolumeMounts", + "terminationGracePeriodSeconds", + "shutdownDrainSeconds", + "shutdownCleanupSeconds", + ] { + assert!( + values.contains(&format!("\n{key}:")), + "{key} must exist in values.yaml" + ); + assert!( + readme.contains(key), + "the packaged README must document the {key} value" + ); + } + + for (key, default) in [ + ("terminationGracePeriodSeconds", "90"), + ("shutdownDrainSeconds", "60"), + ("shutdownCleanupSeconds", "15"), + ] { + let declared = values + .lines() + .find_map(|line| line.strip_prefix(&format!("{key}:"))) + .map(str::trim) + .unwrap_or_else(|| panic!("{key} must be a top-level value")); + assert_eq!( + declared, default, + "values.yaml default for {key} changed; update the packaged README table too" + ); + assert!( + readme.contains(&format!("`{default}`")), + "the README value table must show {key}'s default of {default}" + ); + } +} + +/// Chart, appVersion and the rendered image tag must move together; the release +/// job rewrites all three from one version and CI verifies the published chart +/// against them. +#[test] +fn test_rendered_image_tag_matches_chart_app_version() { + if !helm_available() { + eprintln!("skipping: helm not on PATH"); + return; + } + let chart = std::fs::read_to_string(chart_dir().join("Chart.yaml")).unwrap(); + let app_version = chart + .lines() + .find_map(|line| line.trim().strip_prefix("appVersion:")) + .map(|value| value.trim().trim_matches('"').to_string()) + .expect("Chart.yaml must declare appVersion"); + + let rendered = helm(&["template", "op", chart_dir().to_str().unwrap()]); + let image = rendered + .lines() + .find_map(|line| line.trim().strip_prefix("image:")) + .map(|value| value.trim().trim_matches('"').to_string()) + .expect("the StatefulSet must render a container image"); + + assert_eq!( + image, + format!("untra/operator:{app_version}"), + "the chart must deploy its own appVersion, never a floating tag" + ); +} + +/// Kept honest against the helper above: a chart path that does not exist must +/// not silently pass as "helm unavailable". +#[test] +fn test_chart_directory_exists() { + assert!( + Path::new(&chart_dir()).join("Chart.yaml").exists(), + "charts/operator/Chart.yaml is missing" + ); +} diff --git a/tests/setup_parity.rs b/tests/setup_parity.rs index 584508f1..a03ddfd8 100644 --- a/tests/setup_parity.rs +++ b/tests/setup_parity.rs @@ -208,3 +208,76 @@ fn test_web_parity_scope_cuts_are_explicit() { assert!(web_steps_tsx().contains("Ticket creation is read-only")); assert!(tsx_contains_code("wrapperSteps.has(step)")); } + +/// Coder template parameters are held in `draft.coderParameters` (stable row ids) +/// and only folded into the request payload at submit. Editing the target +/// name or template must not clear them - the earlier version of these handlers +/// wrote `parameters: {}` back into the target on every keystroke. +#[test] +fn test_coder_parameter_rows_survive_target_name_and_template_edits() { + assert!( + tsx_contains_code("coderParameters: current.coderParameters.map("), + "parameter rows must be edited in place by id, not rebuilt from the target" + ); + let carried = WEB_STEPS_TSX + .matches("? current.executionTarget.parameters") + .count(); + assert_eq!( + carried, 2, + "both the target-name and the template handler must carry existing parameters \ + through an edit instead of resetting them to an empty map" + ); + + // The only legitimate empty initialisation is selecting the coder kind for + // the first time; every other site would silently drop entered parameters. + let reset = WEB_STEPS_TSX.matches("parameters: {},").count(); + assert_eq!( + reset, 1, + "only the coder-kind selection may initialise parameters to an empty map" + ); +} + +/// Parameter values can carry credentials-adjacent template input. The review +/// step acknowledges that parameters exist and where they land, but never renders a value. +#[test] +fn test_confirm_step_reports_parameter_count_without_values() { + let confirm = WEB_STEPS_TSX + .split_once("const Confirm: StepComponent") + .expect("steps.tsx must define a Confirm step") + .1 + .split_once("export const STEP_COMPONENTS") + .expect("Confirm must precede the component map") + .0; + + assert!( + confirm.contains("draft.coderParameters.length"), + "the review summary must report how many Coder parameters were entered" + ); + assert!( + confirm.contains("stored in the project configuration"), + "the review summary must say where parameter values are persisted" + ); + assert!( + !confirm.contains("parameter.value") && !confirm.contains(".value}"), + "the review summary must never render a Coder parameter value" + ); +} + +/// Empty and duplicate names are rejected before submit; values are sent verbatim +#[test] +fn test_web_wizard_validates_coder_parameter_names() { + const PAGE_TSX: &str = include_str!("../ui/src/routes/onboarding/OnboardingPage.tsx"); + + assert!( + PAGE_TSX.contains("Coder parameter names cannot be empty."), + "the wizard must reject an empty parameter name" + ); + assert!( + PAGE_TSX.contains("Coder parameter names must be unique."), + "the wizard must reject duplicate parameter names" + ); + assert!( + PAGE_TSX.contains("[name.trim(), value]"), + "parameter names are trimmed but values must be submitted verbatim" + ); +} diff --git a/ui/src/Layout.tsx b/ui/src/Layout.tsx index 1cab3433..c9cff0e0 100644 --- a/ui/src/Layout.tsx +++ b/ui/src/Layout.tsx @@ -11,6 +11,10 @@ import type { SectionDto } from "./api-client"; import { OperatorApi, setCsrfToken } from "./api-client"; import { useHost } from "./host"; +function navLinkClassName({ isActive }: { isActive: boolean }): string { + return isActive ? `${styles.navLink} ${styles.active}` : styles.navLink; +} + // The "Status" group mirrors the canonical section order shared with the TUI and // VS Code extension (the SectionId enum in src/ui/status_panel.rs) and reflects // each section's live health from GET /api/v1/sections. A section whose @@ -43,13 +47,7 @@ function NavRow({ concept, section }: { concept: Concept; section?: SectionDto } } return ( - - isActive ? `${styles.navLink} ${styles.active}` : styles.navLink - } - > + {inner} ); diff --git a/ui/src/components/KanbanBoard.tsx b/ui/src/components/KanbanBoard.tsx index a50d5c0a..0ab97f6a 100644 --- a/ui/src/components/KanbanBoard.tsx +++ b/ui/src/components/KanbanBoard.tsx @@ -1,3 +1,4 @@ +import { useCallback } from "react"; import type { KanbanBoardResponse } from "@operator/bindings/KanbanBoardResponse"; import type { KanbanTicketCard } from "@operator/bindings/KanbanTicketCard"; import { useRightPanel } from "../right-panel"; @@ -16,8 +17,10 @@ import styles from "./KanbanBoard.module.css"; */ export function KanbanBoard({ board }: { board: KanbanBoardResponse }) { const { open } = useRightPanel(); - const openTicket = (ticket: KanbanTicketCard) => - open(, ticket.id); + const openTicket = useCallback( + (ticket: KanbanTicketCard) => open(, ticket.id), + [open], + ); const inProgress = [...board.running, ...board.awaiting]; return ( @@ -60,6 +63,7 @@ function Card({ ticket: KanbanTicketCard; onOpen?: (ticket: KanbanTicketCard) => void; }) { + const handleOpen = useCallback(() => onOpen?.(ticket), [onOpen, ticket]); const inner = ( <>
@@ -86,7 +90,7 @@ function Card({ type="button" className={`${styles.card} ${styles.cardClickable}`} data-priority={priorityKey(ticket.priority)} - onClick={() => onOpen(ticket)} + onClick={handleOpen} title="Open ticket detail" > {inner} diff --git a/ui/src/right-panel.tsx b/ui/src/right-panel.tsx index 6a451926..3b9a1ffc 100644 --- a/ui/src/right-panel.tsx +++ b/ui/src/right-panel.tsx @@ -29,14 +29,17 @@ export function RightPanelProvider({ children }: { children: ReactNode }) { const [content, setContent] = useState(null); const [title, setTitle] = useState(null); - const open = useCallback((node: ReactNode, t?: string) => { - setContent(node); - setTitle(t ?? null); - }, []); + const open = useCallback( + (node: ReactNode, t?: string) => { + setContent(node); + setTitle(t ?? null); + }, + [setContent, setTitle], + ); const close = useCallback(() => { setContent(null); setTitle(null); - }, []); + }, [setContent, setTitle]); const value = useMemo(() => ({ content, title, open, close }), [content, title, open, close]); diff --git a/ui/src/routes/DevicePage.tsx b/ui/src/routes/DevicePage.tsx index 6426d5c4..82009dcf 100644 --- a/ui/src/routes/DevicePage.tsx +++ b/ui/src/routes/DevicePage.tsx @@ -73,7 +73,7 @@ export function DevicePage() { setUserCode(e.target.value.toUpperCase())} + onChange={(event) => setUserCode(event.target.value.toUpperCase())} placeholder="XXXX-XXXX" required /> diff --git a/ui/src/routes/ForgotPasswordPage.tsx b/ui/src/routes/ForgotPasswordPage.tsx index 2f22fcb4..b51114fa 100644 --- a/ui/src/routes/ForgotPasswordPage.tsx +++ b/ui/src/routes/ForgotPasswordPage.tsx @@ -10,7 +10,6 @@ export function ForgotPasswordPage() { const [username, setUsername] = useState(""); const [message, setMessage] = useState(null); const [busy, setBusy] = useState(false); - async function submit(event: React.SubmitEvent) { event.preventDefault(); setBusy(true); diff --git a/ui/src/routes/LoginPage.tsx b/ui/src/routes/LoginPage.tsx index 02cf2da6..6fb9fdb9 100644 --- a/ui/src/routes/LoginPage.tsx +++ b/ui/src/routes/LoginPage.tsx @@ -72,7 +72,7 @@ export function LoginPage() { autoComplete="username" maxLength={MAX_USERNAME_LENGTH} value={username} - onChange={(e) => setUsername(e.target.value)} + onChange={(event) => setUsername(event.target.value)} required /> @@ -85,7 +85,7 @@ export function LoginPage() { autoComplete="current-password" maxLength={MAX_PASSWORD_LENGTH} value={password} - onChange={(e) => setPassword(e.target.value)} + onChange={(event) => setPassword(event.target.value)} required /> diff --git a/ui/src/routes/ModelProvidersPage.tsx b/ui/src/routes/ModelProvidersPage.tsx index 1dc8f70c..de63f30a 100644 --- a/ui/src/routes/ModelProvidersPage.tsx +++ b/ui/src/routes/ModelProvidersPage.tsx @@ -109,24 +109,35 @@ export function ModelProvidersPage() { useEffect(refreshDelegators, [refreshDelegators]); - const connectGateway = async (kind: ModelServerKindEntry) => { - setError(null); - try { - await api.createModelServer({ - name: kind.slug, - kind: kind.slug, - base_url: kind.default_base_url ?? null, - api_key_env: kind.default_api_key_env ?? null, - extra_env: {}, - display_name: kind.display_name, - }); - setNotice(`Declared "${kind.slug}". Re-probingโ€ฆ`); - const r = await api.providerModels(kind.slug); - setProbes((p) => ({ ...p, [kind.slug]: r })); - } catch (e) { - setError(e instanceof Error ? e.message : "Failed to connect provider"); - } - }; + const connectGateway = useCallback( + async (kind: ModelServerKindEntry) => { + setError(null); + try { + await api.createModelServer({ + name: kind.slug, + kind: kind.slug, + base_url: kind.default_base_url ?? null, + api_key_env: kind.default_api_key_env ?? null, + extra_env: {}, + display_name: kind.display_name, + }); + setNotice(`Declared "${kind.slug}". Re-probingโ€ฆ`); + const r = await api.providerModels(kind.slug); + setProbes((p) => ({ ...p, [kind.slug]: r })); + } catch (e) { + setError(e instanceof Error ? e.message : "Failed to connect provider"); + } + }, + [api], + ); + + const handleDelegatorCreated = useCallback( + (name: string) => { + setNotice(`Created delegator "${name}".`); + refreshDelegators(); + }, + [refreshDelegators], + ); const firstParty = useMemo(() => kinds.filter((k) => k.category === "first-party"), [kinds]); const gateways = useMemo(() => kinds.filter((k) => k.category === "gateway"), [kinds]); @@ -167,10 +178,7 @@ export function ModelProvidersPage() { kinds={kinds} probes={probes} detectedTools={detectedTools} - onCreated={(name) => { - setNotice(`Created delegator "${name}".`); - refreshDelegators(); - }} + onCreated={handleDelegatorCreated} onError={setError} /> diff --git a/ui/src/routes/onboarding/OnboardingPage.tsx b/ui/src/routes/onboarding/OnboardingPage.tsx index 23965b80..d211b5ad 100644 --- a/ui/src/routes/onboarding/OnboardingPage.tsx +++ b/ui/src/routes/onboarding/OnboardingPage.tsx @@ -1,4 +1,4 @@ -import { useEffect, useMemo, useState } from "react"; +import { useCallback, useEffect, useMemo, useState } from "react"; import { useNavigate } from "react-router-dom"; import type { SetupStep } from "@operator/bindings/SetupStep"; import type { SetupStatusResponse } from "../../api-client"; @@ -91,11 +91,14 @@ export function OnboardingPage() { const current = steps.find((step) => step.slug === walk[currentIndex]); const Step = current ? STEP_COMPONENTS[current.slug] : null; - function addExport(value: string) { - setExports((currentExports) => - currentExports.includes(value) ? currentExports : [...currentExports, value], - ); - } + const addExport = useCallback( + (value: string) => { + setExports((currentExports) => + currentExports.includes(value) ? currentExports : [...currentExports, value], + ); + }, + [setExports], + ); function next() { if (!current) { diff --git a/ui/src/routes/onboarding/steps.tsx b/ui/src/routes/onboarding/steps.tsx index 63ef332a..b70365ea 100644 --- a/ui/src/routes/onboarding/steps.tsx +++ b/ui/src/routes/onboarding/steps.tsx @@ -1,4 +1,4 @@ -import { useEffect, useMemo, useState } from "react"; +import { useCallback, useEffect, useMemo, useState } from "react"; import type { KanbanProviderKind } from "@operator/bindings/KanbanProviderKind"; import type { SetupStep } from "@operator/bindings/SetupStep"; import type { StepComponent, StepProps } from "./types"; @@ -7,25 +7,33 @@ import styles from "./OnboardingPage.module.css"; const TASK_FIELDS = ["priority", "points", "user_story"] as const; const WRAPPERS = ["tmux", "vscode", "cmux", "zellij"] as const; const KANBAN_KINDS = ["jira", "linear", "github", "openspec"] as const; +const COLLECTION_SOURCES = [ + ["simple", "Simple"], + ["dev_kanban", "Development"], + ["devops_kanban", "DevOps"], + ["custom", "Hosted collections"], +] as const; function Intro({ children }: { children: React.ReactNode }) { return
{children}
; } -function Choice({ +function Choice({ selected, - onClick, + value, + onSelect, children, }: { selected: boolean; - onClick: () => void; + value: Value; + onSelect: (value: Value) => void; children: React.ReactNode; }) { return ( @@ -86,6 +94,11 @@ function KanbanInfo({ api, addExport }: StepProps) { .catch((error: Error) => setMessage(error.message)); }, [api]); + const selectProvider = useCallback( + (slug: string) => setProvider(KANBAN_KINDS.find((kind) => kind === slug) ?? ""), + [setProvider], + ); + const credentials = () => ({ provider: provider as KanbanProviderKind, jira: provider === "jira" ? { domain, email, api_token: token } : null, @@ -250,7 +263,8 @@ function KanbanInfo({ api, addExport }: StepProps) { setProvider(KANBAN_KINDS.find((kind) => kind === item.slug) ?? "")} + value={item.slug} + onSelect={selectProvider} > {item.display_name} {item.description} @@ -386,6 +400,16 @@ function ModelServer({ api, integrations, draft, setDraft }: StepProps) { const env = kinds.find((kind) => kind.slug === slug)?.default_api_key_env; return env ? [`export ${env}=""`] : []; }); + const toggleProvider = useCallback( + (provider: string) => + setDraft((current) => ({ + ...current, + modelServers: current.modelServers.includes(provider) + ? current.modelServers.filter((slug) => slug !== provider) + : [...current.modelServers, provider], + })), + [setDraft], + ); return (

Model providers

@@ -398,14 +422,8 @@ function ModelServer({ api, integrations, draft, setDraft }: StepProps) { - setDraft((current) => ({ - ...current, - modelServers: current.modelServers.includes(entry.slug) - ? current.modelServers.filter((slug) => slug !== entry.slug) - : [...current.modelServers, entry.slug], - })) - } + value={entry.slug} + onSelect={toggleProvider} > {entry.label} {probes[entry.slug] ?? "checkingโ€ฆ"} @@ -470,7 +488,8 @@ function GitProvider({ api, addExport }: StepProps) { setSelected(item.slug)} + value={item.slug} + onSelect={setSelected} > {item.label} @@ -510,273 +529,309 @@ function GitProvider({ api, addExport }: StepProps) { ); } -const CollectionSource: StepComponent = ({ draft, setDraft }) => ( - -

Issue type collection

-
- {( - [ - ["simple", "Simple"], - ["dev_kanban", "Development"], - ["devops_kanban", "DevOps"], - ["custom", "Hosted collections"], - ] as const - ).map(([value, label]) => ( - setDraft((current) => ({ ...current, preset: value }))} - > - {label} - - ))} -
- -); +const CollectionSource: StepComponent = ({ draft, setDraft }) => { + const selectPreset = useCallback( + (preset: (typeof COLLECTION_SOURCES)[number][0]) => + setDraft((current) => ({ ...current, preset })), + [setDraft], + ); + return ( + +

Issue type collection

+
+ {COLLECTION_SOURCES.map(([value, label]) => ( + + {label} + + ))} +
+ + ); +}; -const HostedCollections: StepComponent = ({ collections, draft, setDraft }) => ( - -

Hosted collections

-

Select one or more. The checksum locks initialization to the version you reviewed.

-
- {collections.map((item) => ( - - setDraft((current) => ({ - ...current, - hostedCollectionIds: current.hostedCollectionIds.includes(item.id) - ? current.hostedCollectionIds.filter((id) => id !== item.id) - : [...current.hostedCollectionIds, item.id], - })) - } - > - {item.name} - {item.description} - {item.types.join(", ")} - - ))} -
- -); +const HostedCollections: StepComponent = ({ collections, draft, setDraft }) => { + const toggleCollection = useCallback( + (id: string) => + setDraft((current) => ({ + ...current, + hostedCollectionIds: current.hostedCollectionIds.includes(id) + ? current.hostedCollectionIds.filter((collectionId) => collectionId !== id) + : [...current.hostedCollectionIds, id], + })), + [setDraft], + ); + return ( + +

Hosted collections

+

Select one or more. The checksum locks initialization to the version you reviewed.

+
+ {collections.map((item) => ( + + {item.name} + {item.description} + {item.types.join(", ")} + + ))} +
+ + ); +}; -const TaskFieldConfig: StepComponent = ({ draft, setDraft }) => ( - -

Optional task fields

-
- {TASK_FIELDS.map((field) => ( +const TaskFieldConfig: StepComponent = ({ draft, setDraft }) => { + const toggleTaskField = useCallback( + (field: string) => + setDraft((current) => ({ + ...current, + taskFields: current.taskFields.includes(field) + ? current.taskFields.filter((item) => item !== field) + : [...current.taskFields, field], + })), + [setDraft], + ); + return ( + +

Optional task fields

+
+ {TASK_FIELDS.map((field) => ( + + {field.replace("_", " ")} + + ))} +
+ + ); +}; + +const SessionWrapperChoice: StepComponent = ({ draft, setDraft }) => { + const selectWrapper = useCallback( + (wrapper: (typeof WRAPPERS)[number]) => + setDraft((current) => ({ + ...current, + wrapper, + executionTarget: + wrapper === "zellij" && current.executionTarget.kind === "coder" + ? { kind: "local" } + : current.executionTarget, + })), + [setDraft], + ); + return ( + +

Session wrapper

+
+ {WRAPPERS.map((wrapper) => ( + + {wrapper} + + ))} +
+ + ); +}; + +const ExecutionTarget: StepComponent = ({ draft, setDraft }) => { + const selectExecutionTarget = useCallback( + (kind: "local" | "coder") => + setDraft((current) => ({ + ...current, + useWorktrees: kind === "coder" ? false : current.useWorktrees, + executionTarget: + kind === "local" + ? { kind: "local" } + : { + kind: "coder", + name: "coder-agents", + template: "", + parameters: {}, + }, + })), + [setDraft], + ); + return ( + +

Execution target

+
- setDraft((current) => ({ - ...current, - taskFields: current.taskFields.includes(field) - ? current.taskFields.filter((item) => item !== field) - : [...current.taskFields, field], - })) - } + selected={draft.executionTarget.kind === "local"} + value="local" + onSelect={selectExecutionTarget} > - {field.replace("_", " ")} + Local + Run beside Operator - ))} -
- -); - -const SessionWrapperChoice: StepComponent = ({ draft, setDraft }) => ( - -

Session wrapper

-
- {WRAPPERS.map((wrapper) => ( - setDraft((current) => ({ - ...current, - wrapper, - executionTarget: - wrapper === "zellij" && current.executionTarget.kind === "coder" - ? { kind: "local" } - : current.executionTarget, - })) - } + selected={draft.executionTarget.kind === "coder"} + value="coder" + onSelect={selectExecutionTarget} > - {wrapper} + Coder + One workspace per ticket over SSH - ))} -
- -); - -const ExecutionTarget: StepComponent = ({ draft, setDraft }) => ( - -

Execution target

-
- setDraft((current) => ({ ...current, executionTarget: { kind: "local" } }))} - > - Local - Run beside Operator - - - setDraft((current) => ({ - ...current, - useWorktrees: false, - executionTarget: { - kind: "coder", - name: "coder-agents", - template: "", - parameters: {}, - }, - })) - } - > - Coder - One workspace per ticket over SSH - -
- {draft.executionTarget.kind === "coder" && ( -
- - -
- Template parameters (optional) - {draft.coderParameters.map((parameter, index) => ( -
- - setDraft((current) => ({ - ...current, - coderParameters: current.coderParameters.map((item) => - item.id === parameter.id ? { ...item, name: event.target.value } : item, - ), - })) - } - /> - - setDraft((current) => ({ - ...current, - coderParameters: current.coderParameters.map((item) => - item.id === parameter.id ? { ...item, value: event.target.value } : item, - ), - })) - } - /> - -
- ))} - -
-

Set CODER_URL and CODER_SESSION_TOKEN in the server environment.

- )} - -); + {draft.executionTarget.kind === "coder" && ( +
+ + +
+ Template parameters (optional) + {draft.coderParameters.map((parameter, index) => ( +
+ + setDraft((current) => ({ + ...current, + coderParameters: current.coderParameters.map((item) => + item.id === parameter.id ? { ...item, name: event.target.value } : item, + ), + })) + } + /> + + setDraft((current) => ({ + ...current, + coderParameters: current.coderParameters.map((item) => + item.id === parameter.id ? { ...item, value: event.target.value } : item, + ), + })) + } + /> + +
+ ))} + +
+

Set CODER_URL and CODER_SESSION_TOKEN in the server environment.

+
+ )} + + ); +}; -const WorktreePreference: StepComponent = ({ draft, setDraft }) => ( - -

Git worktrees

-

Coder targets always isolate work remotely, so local worktrees are disabled for them.

-
- setDraft((current) => ({ ...current, useWorktrees: false }))} - > - In-place branches - - setDraft((current) => ({ ...current, useWorktrees: true }))} - > - Per-ticket worktrees - -
- -); +const WorktreePreference: StepComponent = ({ draft, setDraft }) => { + const selectWorktreePreference = useCallback( + (preference: "in-place" | "worktree") => + setDraft((current) => ({ ...current, useWorktrees: preference === "worktree" })), + [setDraft], + ); + return ( + +

Git worktrees

+

Coder targets always isolate work remotely, so local worktrees are disabled for them.

+
+ + In-place branches + + + Per-ticket worktrees + +
+ + ); +}; const AdminPassword: StepComponent = () => ( @@ -850,6 +905,12 @@ const Confirm: StepComponent = ({ status, draft, exports }) => (
{draft.wrapper}
Execution
{draft.executionTarget.kind}
+ {draft.executionTarget.kind === "coder" && ( + <> +
Coder parameters
+
{draft.coderParameters.length} stored in the project configuration
+ + )}
Worktrees
{draft.useWorktrees ? "enabled" : "disabled"}
diff --git a/ui/src/theme.ts b/ui/src/theme.ts index f21b5b1f..729fd782 100644 --- a/ui/src/theme.ts +++ b/ui/src/theme.ts @@ -39,7 +39,7 @@ export function useTheme(): { theme: Theme; toggleTheme: () => void } { const toggleTheme = useCallback(() => { setTheme((prev) => (prev === "dark" ? "light" : "dark")); - }, []); + }, [setTheme]); return { theme, toggleTheme }; } diff --git a/vscode-extension/src/webhook-server.ts b/vscode-extension/src/webhook-server.ts index 18aec7ae..89783a75 100644 --- a/vscode-extension/src/webhook-server.ts +++ b/vscode-extension/src/webhook-server.ts @@ -22,7 +22,7 @@ import type { SessionInfo, } from "./types"; -const VERSION = "0.2.9"; +const VERSION = "0.2.10"; /** * HTTP server for operator <-> extension communication diff --git a/vscode-extension/webview-ui/App.tsx b/vscode-extension/webview-ui/App.tsx index 5ea3bb63..e529304a 100644 --- a/vscode-extension/webview-ui/App.tsx +++ b/vscode-extension/webview-ui/App.tsx @@ -16,6 +16,34 @@ import type { JiraConfig } from "../src/generated/JiraConfig"; import type { LinearConfig } from "../src/generated/LinearConfig"; import type { ProjectSyncConfig } from "../src/generated/ProjectSyncConfig"; +function browseFolder(field: string): void { + postMessage({ type: "browseFolder", field }); +} + +function openFile(filePath: string): void { + postMessage({ type: "openFile", filePath }); +} + +function startSetup(): void { + postMessage({ type: "openWalkthrough" }); +} + +function detectTools(): void { + postMessage({ type: "detectLlmTools" }); +} + +function getExternalIssueTypes(provider: string, domain: string, projectKey: string): void { + postMessage({ type: "getExternalIssueTypes", provider, domain, projectKey }); +} + +function getKanbanStatuses(provider: string, projectKey: string): void { + postMessage({ type: "getKanbanStatuses", provider, projectKey }); +} + +function openOperatorUi(route: "issuetypes" | "projects"): void { + postMessage({ type: "openOperatorUi", route }); +} + export function App() { const [config, setConfig] = useState(null); const [error, setError] = useState(null); @@ -126,60 +154,38 @@ export function App() { return cleanup; }, []); - const handleUpdate = useCallback((section: string, key: string, value: unknown) => { - postMessage({ type: "updateConfig", section, key, value }); + const handleUpdate = useCallback( + (section: string, key: string, value: unknown) => { + postMessage({ type: "updateConfig", section, key, value }); - // Optimistic update for responsiveness - setConfig((prev) => { - if (!prev) { - return prev; - } - return applyUpdate(prev, section, key, value); - }); - }, []); - - const handleBrowseFolder = useCallback((field: string) => { - postMessage({ type: "browseFolder", field }); - }, []); - - const handleOpenFile = useCallback((filePath: string) => { - postMessage({ type: "openFile", filePath }); - }, []); - - const handleStartSetup = useCallback(() => { - postMessage({ type: "openWalkthrough" }); - }, []); - - const handleValidateJira = useCallback((domain: string, email: string, apiToken: string) => { - setValidatingJira(true); - setJiraResult(null); - postMessage({ type: "validateJira", domain, email, apiToken }); - }, []); - - const handleValidateLinear = useCallback((apiKey: string) => { - setValidatingLinear(true); - setLinearResult(null); - postMessage({ type: "validateLinear", apiKey }); - }, []); - - const handleDetectTools = useCallback(() => { - postMessage({ type: "detectLlmTools" }); - }, []); - - const handleGetExternalIssueTypes = useCallback( - (provider: string, domain: string, projectKey: string) => { - postMessage({ type: "getExternalIssueTypes", provider, domain, projectKey }); + // Optimistic update for responsiveness + setConfig((prev) => { + if (!prev) { + return prev; + } + return applyUpdate(prev, section, key, value); + }); }, - [], + [setConfig], ); - const handleGetKanbanStatuses = useCallback((provider: string, projectKey: string) => { - postMessage({ type: "getKanbanStatuses", provider, projectKey }); - }, []); + const handleValidateJira = useCallback( + (domain: string, email: string, apiToken: string) => { + setValidatingJira(true); + setJiraResult(null); + postMessage({ type: "validateJira", domain, email, apiToken }); + }, + [setJiraResult, setValidatingJira], + ); - const handleOpenOperatorUi = useCallback((route: "issuetypes" | "projects") => { - postMessage({ type: "openOperatorUi", route }); - }, []); + const handleValidateLinear = useCallback( + (apiKey: string) => { + setValidatingLinear(true); + setLinearResult(null); + postMessage({ type: "validateLinear", apiKey }); + }, + [setLinearResult, setValidatingLinear], + ); return ( <> @@ -192,12 +198,12 @@ export function App() { ) : (
onOpenFile(config.config_path), + [config.config_path, onOpenFile], + ); + const handleOpenProjects = useCallback(() => onOpenOperatorUi("projects"), [onOpenOperatorUi]); const navItems: NavItem[] = useMemo( () => [ @@ -97,7 +102,7 @@ export function ConfigPage({
diff --git a/vscode-extension/webview-ui/components/SidebarNav.tsx b/vscode-extension/webview-ui/components/SidebarNav.tsx index 3fa99d41..f6812473 100644 --- a/vscode-extension/webview-ui/components/SidebarNav.tsx +++ b/vscode-extension/webview-ui/components/SidebarNav.tsx @@ -1,4 +1,4 @@ -import React, { useEffect, useState, useCallback } from "react"; +import React, { useEffect, useState } from "react"; import { OperatorBrand } from "./OperatorBrand"; export interface NavItem { @@ -12,18 +12,22 @@ interface SidebarNavProps { scrollContainerRef: React.RefObject; } +function scrollToItem(item: NavItem): void { + if (item.disabled) { + return; + } + document.getElementById(item.id)?.scrollIntoView({ behavior: "smooth", block: "start" }); +} + export function SidebarNav({ items, scrollContainerRef }: SidebarNavProps) { const [activeId, setActiveId] = useState(items[0]?.id ?? ""); - const handleClick = useCallback((item: NavItem) => { - if (item.disabled) { - return; - } - const element = document.getElementById(item.id); - if (element) { - element.scrollIntoView({ behavior: "smooth", block: "start" }); + function handleNavigationClick(event: React.MouseEvent) { + const item = items.find(({ id }) => id === event.currentTarget.dataset.sectionId); + if (item) { + scrollToItem(item); } - }, []); + } useEffect(() => { const container = scrollContainerRef.current; @@ -71,7 +75,8 @@ export function SidebarNav({ items, scrollContainerRef }: SidebarNavProps) { className="op-nav-item" data-selected={activeId === item.id && !item.disabled ? "true" : undefined} disabled={item.disabled} - onClick={() => handleClick(item)} + data-section-id={item.id} + onClick={handleNavigationClick} > {item.label} diff --git a/vscode-extension/webview-ui/components/kanban/MappingRow.tsx b/vscode-extension/webview-ui/components/kanban/MappingRow.tsx index 04e57980..4f104b51 100644 --- a/vscode-extension/webview-ui/components/kanban/MappingRow.tsx +++ b/vscode-extension/webview-ui/components/kanban/MappingRow.tsx @@ -1,4 +1,4 @@ -import React from "react"; +import React, { useCallback } from "react"; import { SelectInput } from "../primitives"; import type { ExternalIssueTypeSummary, IssueTypeSummary } from "../../types/messages"; @@ -25,6 +25,10 @@ export function MappingRow({ }: MappingRowProps) { const effectiveKey = selectedKey ?? autoMatchedKey; const isOverride = selectedKey !== null && selectedKey !== autoMatchedKey; + const handleSelectionChange = useCallback( + (event: React.ChangeEvent) => onSelect(external.name, event.target.value), + [external.name, onSelect], + ); return (
@@ -46,10 +50,7 @@ export function MappingRow({ {/* Operator type selector */}
- onSelect(external.name, e.target.value)} - > + {operatorTypes.map((ot) => (
- onUpdate(sectionKey, "enabled", e.target.checked)} - label="Enabled" - /> +
@@ -96,7 +129,7 @@ export function ProviderCard({ {isJira ? `${domain} ยท ${jiraConfig?.email || "no email"}` : domain} -
@@ -110,7 +143,7 @@ export function ProviderCard({ onUpdate(sectionKey, "domain", e.target.value)} + onChange={handleDomainChange} placeholder="your-org.atlassian.net" disabled={!enabled} helperText="Jira Cloud instance domain" @@ -118,14 +151,14 @@ export function ProviderCard({ onUpdate(sectionKey, "email", e.target.value)} + onChange={handleEmailChange} placeholder="you@example.com" disabled={!enabled} /> onUpdate(sectionKey, "api_key_env", e.target.value)} + onChange={handleApiKeyEnvChange} disabled={!enabled} /> @@ -134,13 +167,13 @@ export function ProviderCard({ onUpdate(sectionKey, "team_id", e.target.value)} + onChange={handleDomainChange} disabled={!enabled} /> onUpdate(sectionKey, "api_key_env", e.target.value)} + onChange={handleApiKeyEnvChange} disabled={!enabled} /> @@ -151,20 +184,14 @@ export function ProviderCard({ type="password" label={isJira ? "API Token" : "API Key"} value={apiToken} - onChange={(e) => setApiToken(e.target.value)} + onChange={handleApiTokenChange} placeholder={isJira ? "Paste token to validate" : "lin_api_xxxxx"} disabled={!enabled} style={{ flexGrow: 1 }} />
@@ -217,12 +244,7 @@ export function ProviderCard({ {/* Add project shortcut */}
- { - onUpdate(sectionKey, `projects.${key}.collection_name`, ""); - }} - /> +
@@ -233,12 +255,20 @@ export function ProviderCard({ function AddProjectInput({ disabled, onAdd }: { disabled: boolean; onAdd: (key: string) => void }) { const [value, setValue] = useState(""); + const handleValueChange = useCallback( + (event: React.ChangeEvent) => setValue(event.target.value.toUpperCase()), + [setValue], + ); + const handleAdd = useCallback(() => { + onAdd(value.trim()); + setValue(""); + }, [onAdd, value]); return (
setValue(e.target.value.toUpperCase())} + onChange={handleValueChange} placeholder="PROJ" disabled={disabled} style={{ flex: 1 }} @@ -247,10 +277,7 @@ function AddProjectInput({ disabled, onAdd }: { disabled: boolean; onAdd: (key: size="small" variant="outlined" disabled={disabled || !value.trim()} - onClick={() => { - onAdd(value.trim()); - setValue(""); - }} + onClick={handleAdd} > Add diff --git a/vscode-extension/webview-ui/components/sections/CodingAgentsSection.tsx b/vscode-extension/webview-ui/components/sections/CodingAgentsSection.tsx index 989e3e7a..cdeb7113 100644 --- a/vscode-extension/webview-ui/components/sections/CodingAgentsSection.tsx +++ b/vscode-extension/webview-ui/components/sections/CodingAgentsSection.tsx @@ -1,4 +1,4 @@ -import React from "react"; +import React, { useCallback } from "react"; import { Button, Chip } from "../primitives"; import { SectionHeader } from "../SectionHeader"; import type { AgentsConfig } from "../../../src/generated/AgentsConfig"; @@ -50,6 +50,26 @@ export function CodingAgentsSection({ const stepTimeout = Number(agents.step_timeout); const silenceThreshold = Number(agents.silence_threshold); const detected = llm_tools.detected; + const handleMaxParallelChange = useCallback( + (event: React.ChangeEvent) => + onUpdate("agents", "max_parallel", Number.parseInt(event.target.value, 10) || 1), + [onUpdate], + ); + const handleGenerationTimeoutChange = useCallback( + (event: React.ChangeEvent) => + onUpdate("agents", "generation_timeout_secs", Number.parseInt(event.target.value, 10) || 300), + [onUpdate], + ); + const handleStepTimeoutChange = useCallback( + (event: React.ChangeEvent) => + onUpdate("agents", "step_timeout", Number.parseInt(event.target.value, 10) || 1800), + [onUpdate], + ); + const handleSilenceThresholdChange = useCallback( + (event: React.ChangeEvent) => + onUpdate("agents", "silence_threshold", Number.parseInt(event.target.value, 10) || 30), + [onUpdate], + ); return (
@@ -101,9 +121,7 @@ export function CodingAgentsSection({ value={maxParallel} min={1} max={16} - onChange={(e) => - onUpdate("agents", "max_parallel", Number.parseInt(e.target.value, 10) || 1) - } + onChange={handleMaxParallelChange} helperText="Maximum number of agents running simultaneously" /> @@ -112,13 +130,7 @@ export function CodingAgentsSection({ value={generationTimeout} min={30} max={3600} - onChange={(e) => - onUpdate( - "agents", - "generation_timeout_secs", - Number.parseInt(e.target.value, 10) || 300, - ) - } + onChange={handleGenerationTimeoutChange} helperText="Timeout for each agent generation step" /> @@ -127,9 +139,7 @@ export function CodingAgentsSection({ value={stepTimeout} min={60} max={7200} - onChange={(e) => - onUpdate("agents", "step_timeout", Number.parseInt(e.target.value, 10) || 1800) - } + onChange={handleStepTimeoutChange} helperText="Maximum seconds a step can run before timing out" /> @@ -138,9 +148,7 @@ export function CodingAgentsSection({ value={silenceThreshold} min={5} max={300} - onChange={(e) => - onUpdate("agents", "silence_threshold", Number.parseInt(e.target.value, 10) || 30) - } + onChange={handleSilenceThresholdChange} helperText="Seconds of silence before considering agent awaiting input" />
diff --git a/vscode-extension/webview-ui/components/sections/GitRepositoriesSection.tsx b/vscode-extension/webview-ui/components/sections/GitRepositoriesSection.tsx index 3139dafc..51891d60 100644 --- a/vscode-extension/webview-ui/components/sections/GitRepositoriesSection.tsx +++ b/vscode-extension/webview-ui/components/sections/GitRepositoriesSection.tsx @@ -1,4 +1,4 @@ -import React from "react"; +import React, { useCallback } from "react"; import { TextInput, SelectInput, Toggle } from "../primitives"; import { SectionHeader } from "../SectionHeader"; import type { GitConfig } from "../../../src/generated/GitConfig"; @@ -14,6 +14,31 @@ export function GitRepositoriesSection({ git, onUpdate }: GitRepositoriesSection const githubTokenEnv = git.github.token_env; const branchFormat = git.branch_format; const useWorktrees = git.use_worktrees; + const handleProviderChange = useCallback( + (event: React.ChangeEvent) => + onUpdate("git", "provider", event.target.value), + [onUpdate], + ); + const handleGithubEnabledChange = useCallback( + (event: React.ChangeEvent) => + onUpdate("git.github", "enabled", event.target.checked), + [onUpdate], + ); + const handleGithubTokenChange = useCallback( + (event: React.ChangeEvent) => + onUpdate("git.github", "token_env", event.target.value), + [onUpdate], + ); + const handleBranchFormatChange = useCallback( + (event: React.ChangeEvent) => + onUpdate("git", "branch_format", event.target.value), + [onUpdate], + ); + const handleWorktreesChange = useCallback( + (event: React.ChangeEvent) => + onUpdate("git", "use_worktrees", event.target.checked), + [onUpdate], + ); return (
@@ -27,7 +52,7 @@ export function GitRepositoriesSection({ git, onUpdate }: GitRepositoriesSection onUpdate("git", "provider", e.target.value)} + onChange={handleProviderChange} > @@ -37,14 +62,14 @@ export function GitRepositoriesSection({ git, onUpdate }: GitRepositoriesSection onUpdate("git.github", "enabled", e.target.checked)} + onChange={handleGithubEnabledChange} label="GitHub integration enabled" /> onUpdate("git.github", "token_env", e.target.value)} + onChange={handleGithubTokenChange} placeholder="GITHUB_TOKEN" helperText="Name of the environment variable containing your GitHub personal access token" disabled={!githubEnabled} @@ -53,14 +78,14 @@ export function GitRepositoriesSection({ git, onUpdate }: GitRepositoriesSection onUpdate("git", "branch_format", e.target.value)} + onChange={handleBranchFormatChange} placeholder="{type}/{ticket_id}-{slug}" helperText="Template for branch names. Variables: {type}, {ticket_id}, {slug}" /> onUpdate("git", "use_worktrees", e.target.checked)} + onChange={handleWorktreesChange} label="Use git worktrees for parallel agent branches" />
diff --git a/vscode-extension/webview-ui/components/sections/KanbanProvidersSection.tsx b/vscode-extension/webview-ui/components/sections/KanbanProvidersSection.tsx index 90732e09..f3e112a6 100644 --- a/vscode-extension/webview-ui/components/sections/KanbanProvidersSection.tsx +++ b/vscode-extension/webview-ui/components/sections/KanbanProvidersSection.tsx @@ -1,4 +1,4 @@ -import React from "react"; +import React, { useCallback } from "react"; import { SectionHeader } from "../SectionHeader"; import { LinkOutCard } from "../LinkOutCard"; import { ProviderCard } from "../kanban/ProviderCard"; @@ -73,9 +73,13 @@ export function KanbanProvidersSection({ const defaultLinearTeam = "default-team"; // Viewing an issue type now links out to the hosted Operator UI. - const handleViewIssueType = () => { + const handleViewIssueType = useCallback(() => { onOpenOperatorUi("issuetypes"); - }; + }, [onOpenOperatorUi]); + const handleOpenIssueTypes = useCallback( + () => onOpenOperatorUi("issuetypes"), + [onOpenOperatorUi], + ); return (
@@ -174,7 +178,7 @@ export function KanbanProvidersSection({ id="section-issuetypes" title="Issue Types & Collections" description="Create and manage issue types and collections in the Operator UI." - onOpen={() => onOpenOperatorUi("issuetypes")} + onOpen={handleOpenIssueTypes} />
)} diff --git a/vscode-extension/webview-ui/components/sections/ModelProvidersSection.tsx b/vscode-extension/webview-ui/components/sections/ModelProvidersSection.tsx index 7e56ba28..4874c043 100644 --- a/vscode-extension/webview-ui/components/sections/ModelProvidersSection.tsx +++ b/vscode-extension/webview-ui/components/sections/ModelProvidersSection.tsx @@ -18,6 +18,13 @@ interface ModelProvidersSectionProps { type ProbeMap = Record; +function connectProvider(event: React.MouseEvent): void { + const slug = event.currentTarget.dataset.providerSlug; + if (slug) { + postMessage({ type: "connectProvider", slug }); + } +} + function DismissableAlert({ severity, onClose, @@ -45,6 +52,8 @@ export function ModelProvidersSection({ detectedTools, apiReachable }: ModelProv const [delegators, setDelegators] = useState([]); const [error, setError] = useState(null); const [notice, setNotice] = useState(null); + const dismissError = useCallback(() => setError(null), [setError]); + const dismissNotice = useCallback(() => setNotice(null), [setNotice]); const load = useCallback(() => { if (apiReachable) { @@ -126,12 +135,12 @@ export function ModelProvidersSection({ detectedTools, apiReachable }: ModelProv )} {error && ( - setError(null)}> + {error} )} {notice && ( - setNotice(null)}> + {notice} )} @@ -203,10 +212,7 @@ function ProviderGroup({ {conn.label === "not connected" && k.connectable && !k.is_builtin && ( - )} @@ -245,7 +251,7 @@ function CreateDelegatorForm({ const probe = provider ? probes[provider] : undefined; const liveModels = probe?.reachable ? probe.models : []; - const submit = () => { + const submit = useCallback(() => { if (!selectedTool || !provider || !model) { return; } @@ -264,7 +270,27 @@ function CreateDelegatorForm({ }); setName(""); setModel(""); - }; + }, [model, name, provider, selectedTool]); + const handleToolChange = useCallback( + (event: React.ChangeEvent) => setTool(event.target.value), + [setTool], + ); + const handleProviderChange = useCallback( + (event: React.ChangeEvent) => { + setProvider(event.target.value); + setModel(""); + }, + [setModel, setProvider], + ); + const handleModelChange = useCallback( + (event: React.ChangeEvent) => + setModel(event.target.value), + [setModel], + ); + const handleNameChange = useCallback( + (event: React.ChangeEvent) => setName(event.target.value), + [setName], + ); return (
@@ -272,11 +298,7 @@ function CreateDelegatorForm({ Create delegator - pair a tool with a connected provider and a live model.

- setTool(e.target.value)} - > + {detectedTools.length === 0 && } {detectedTools.map((t) => ( - { - setProvider(e.target.value); - setModel(""); - }} - > + {kinds.map((k) => ( {liveModels.length > 0 ? ( - setModel(e.target.value)}> + {liveModels.map((m) => (