From 02dbdd650ee498cdb7be42bcd27b37b62e82557c Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 23 Feb 2026 01:59:11 +0000 Subject: [PATCH 01/14] feat: add arcane-deploy action for Docker Compose GitOps sync Adds a reusable GitHub Action that automatically deploys Docker Compose stacks to Arcane's container management platform via its GitOps sync API. Supports directory-based compose file discovery, explicit file lists, shared environment variables, and create-or-update semantics (no deletes). https://claude.ai/code/session_01PrmzWtCEcxJTMjxXje3ndd --- .github/actions/arcane-deploy/action.sh | 344 +++++++++++++++++++++++ .github/actions/arcane-deploy/action.yml | 121 ++++++++ README.md | 87 ++++++ 3 files changed, 552 insertions(+) create mode 100755 .github/actions/arcane-deploy/action.sh create mode 100644 .github/actions/arcane-deploy/action.yml diff --git a/.github/actions/arcane-deploy/action.sh b/.github/actions/arcane-deploy/action.sh new file mode 100755 index 0000000..b7dda85 --- /dev/null +++ b/.github/actions/arcane-deploy/action.sh @@ -0,0 +1,344 @@ +#!/bin/bash +set -euo pipefail + +# --- Configuration --- +ARCANE_URL="${INPUT_ARCANE_URL%/}" +API_KEY="${INPUT_ARCANE_API_KEY}" +ENV_ID="${INPUT_ENVIRONMENT_ID}" +COMPOSE_DIR="${INPUT_COMPOSE_DIR:-}" +COMPOSE_FILES_INPUT="${INPUT_COMPOSE_FILES:-}" +REPO_URL="${INPUT_REPOSITORY_URL:-https://github.com/${GITHUB_REPOSITORY}.git}" +REPO_NAME="${INPUT_REPOSITORY_NAME:-${GITHUB_REPOSITORY##*/}}" +BRANCH="${INPUT_BRANCH:-${GITHUB_REF_NAME:-main}}" +AUTH_TYPE="${INPUT_AUTH_TYPE:-http}" +GIT_TOKEN="${INPUT_GIT_TOKEN:-}" +AUTO_SYNC="${INPUT_AUTO_SYNC:-true}" +SYNC_INTERVAL="${INPUT_SYNC_INTERVAL:-5}" +TRIGGER_SYNC="${INPUT_TRIGGER_SYNC:-true}" +ENV_VARS="${INPUT_ENV_VARS:-}" +SYNC_NAME_PREFIX="${INPUT_SYNC_NAME_PREFIX:-${GITHUB_REPOSITORY##*/}}" + +SYNCS_CREATED=0 +SYNCS_UPDATED=0 +REPOSITORY_ID="" + +# --- Logging --- +log_info() { + echo "[Arcane Deploy] $1" +} + +log_error() { + echo "::error::[Arcane Deploy] $1" +} + +# --- API Helper --- +# Make an authenticated API request to Arcane. +# Usage: arcane_api METHOD /path [curl args...] +arcane_api() { + local method="$1" + local path="$2" + shift 2 + + local url="${ARCANE_URL}/api${path}" + local tmp_body + tmp_body=$(mktemp) + + local http_code + http_code=$(curl -s -w "%{http_code}" \ + -X "${method}" \ + -H "X-Api-Key: ${API_KEY}" \ + -H "Content-Type: application/json" \ + -o "${tmp_body}" \ + "$@" \ + "${url}") || true + + if [[ "${http_code}" -ge 400 ]]; then + log_error "API ${method} ${path} failed (HTTP ${http_code})" + cat "${tmp_body}" >&2 + rm -f "${tmp_body}" + return 1 + fi + + cat "${tmp_body}" + rm -f "${tmp_body}" +} + +# --- Compose File Discovery --- +discover_compose_files() { + local files=() + + # From explicit list + if [[ -n "${COMPOSE_FILES_INPUT}" ]]; then + while IFS= read -r file; do + file="${file#"${file%%[![:space:]]*}"}" # trim leading + file="${file%"${file##*[![:space:]]}"}" # trim trailing + [[ -z "${file}" ]] && continue + files+=("${file}") + done <<< "${COMPOSE_FILES_INPUT}" + fi + + # From directory scan + if [[ -n "${COMPOSE_DIR}" ]]; then + local search_dir="${GITHUB_WORKSPACE:-.}/${COMPOSE_DIR}" + if [[ ! -d "${search_dir}" ]]; then + log_error "compose-dir '${COMPOSE_DIR}' does not exist" + return 1 + fi + + while IFS= read -r -d '' file; do + local rel_path="${file#"${GITHUB_WORKSPACE:-.}/"}" + files+=("${rel_path}") + done < <(find "${search_dir}" -maxdepth 2 -type f \( \ + -name "docker-compose.yml" -o \ + -name "docker-compose.yaml" -o \ + -name "compose.yml" -o \ + -name "compose.yaml" \ + \) -print0 | sort -z) + fi + + if [[ ${#files[@]} -eq 0 ]]; then + log_error "No compose files found. Set compose-dir and/or compose-files." + return 1 + fi + + printf '%s\n' "${files[@]}" +} + +# --- Sync Naming --- +# Derive a sync name from a compose file path. +# "stacks/myapp/compose.yml" -> "${prefix}-myapp" +# "compose.yml" (root) -> "${prefix}" +sync_name_from_path() { + local path="$1" + local dir + dir=$(dirname "${path}") + + local name + if [[ "${dir}" == "." ]]; then + name="${SYNC_NAME_PREFIX}" + else + name=$(basename "${dir}") + if [[ "${name}" != "${SYNC_NAME_PREFIX}" ]]; then + name="${SYNC_NAME_PREFIX}-${name}" + fi + fi + + echo "${name}" +} + +# --- Repository Management --- +# Find an existing Arcane git repository by URL, or create one. +ensure_repository() { + echo "::group::Ensuring git repository in Arcane" + log_info "Looking for repository: ${REPO_URL}" + + local repos + repos=$(arcane_api GET "/customize/git-repositories") + + REPOSITORY_ID=$(echo "${repos}" | jq -r \ + --arg url "${REPO_URL}" \ + '[.[] | select(.url == $url)] | first // empty | .id') + + if [[ -n "${REPOSITORY_ID}" ]]; then + log_info "Found existing repository: ${REPOSITORY_ID}" + + # Update credentials so the token stays current + if [[ "${AUTH_TYPE}" == "http" && -n "${GIT_TOKEN}" ]]; then + local update_payload + update_payload=$(jq -n \ + --arg token "${GIT_TOKEN}" \ + '{token: $token}') + + arcane_api PUT "/customize/git-repositories/${REPOSITORY_ID}" \ + -d "${update_payload}" > /dev/null + log_info "Updated repository credentials" + fi + else + log_info "Creating new repository: ${REPO_NAME}" + + local create_payload + create_payload=$(jq -n \ + --arg name "${REPO_NAME}" \ + --arg url "${REPO_URL}" \ + --arg authType "${AUTH_TYPE}" \ + --arg token "${GIT_TOKEN}" \ + '{name: $name, url: $url, authType: $authType, token: $token}') + + local result + result=$(arcane_api POST "/customize/git-repositories" -d "${create_payload}") + + REPOSITORY_ID=$(echo "${result}" | jq -r '.id') + log_info "Created repository: ${REPOSITORY_ID}" + fi + + echo "::endgroup::" +} + +# --- Sync Management --- +# Create or update a gitops sync for a single compose file. +upsert_sync() { + local compose_path="$1" + local sync_name="$2" + local existing_syncs="$3" + + # Match by compose path + repository ID + local existing_id + existing_id=$(echo "${existing_syncs}" | jq -r \ + --arg composePath "${compose_path}" \ + --arg repoId "${REPOSITORY_ID}" \ + '[.[] | select(.composePath == $composePath and .repositoryId == $repoId)] | first // empty | .id') + + local auto_sync_val="false" + [[ "${AUTO_SYNC}" == "true" ]] && auto_sync_val="true" + + if [[ -n "${existing_id}" ]]; then + log_info " Updating sync ${existing_id} (${sync_name})" + + local update_payload + update_payload=$(jq -n \ + --arg name "${sync_name}" \ + --arg branch "${BRANCH}" \ + --arg composePath "${compose_path}" \ + --argjson autoSync "${auto_sync_val}" \ + --argjson syncInterval "${SYNC_INTERVAL}" \ + '{ + name: $name, + branch: $branch, + composePath: $composePath, + autoSync: $autoSync, + syncInterval: $syncInterval + }') + + arcane_api PUT "/environments/${ENV_ID}/gitops-syncs/${existing_id}" \ + -d "${update_payload}" > /dev/null + + SYNCS_UPDATED=$((SYNCS_UPDATED + 1)) + + if [[ "${TRIGGER_SYNC}" == "true" ]]; then + log_info " Triggering sync..." + arcane_api POST "/environments/${ENV_ID}/gitops-syncs/${existing_id}/sync" > /dev/null || true + fi + else + log_info " Creating sync: ${sync_name}" + + local create_payload + create_payload=$(jq -n \ + --arg name "${sync_name}" \ + --arg repositoryId "${REPOSITORY_ID}" \ + --arg branch "${BRANCH}" \ + --arg composePath "${compose_path}" \ + --arg projectName "${sync_name}" \ + --argjson autoSync "${auto_sync_val}" \ + --argjson syncInterval "${SYNC_INTERVAL}" \ + '{ + name: $name, + repositoryId: $repositoryId, + branch: $branch, + composePath: $composePath, + projectName: $projectName, + autoSync: $autoSync, + syncInterval: $syncInterval + }') + + local result + result=$(arcane_api POST "/environments/${ENV_ID}/gitops-syncs" -d "${create_payload}") + + local new_id + new_id=$(echo "${result}" | jq -r '.id') + log_info " Created sync: ${new_id}" + + SYNCS_CREATED=$((SYNCS_CREATED + 1)) + + if [[ "${TRIGGER_SYNC}" == "true" ]]; then + log_info " Triggering sync..." + arcane_api POST "/environments/${ENV_ID}/gitops-syncs/${new_id}/sync" > /dev/null || true + fi + fi +} + +# --- Environment Variables --- +export_env_vars() { + if [[ -z "${ENV_VARS}" ]]; then + return + fi + + echo "::group::Setting shared environment variables" + while IFS= read -r line; do + line="${line#"${line%%[![:space:]]*}"}" # trim leading + line="${line%"${line##*[![:space:]]}"}" # trim trailing + [[ -z "${line}" ]] && continue + [[ "${line}" == \#* ]] && continue + + local key="${line%%=*}" + local value="${line#*=}" + log_info " ${key}=***" + echo "${key}=${value}" >> "${GITHUB_ENV}" + done <<< "${ENV_VARS}" + echo "::endgroup::" +} + +# --- Main --- + +log_info "Arcane Docker Compose Deploy" +log_info "Instance: ${ARCANE_URL}" +log_info "Environment: ${ENV_ID}" +log_info "Repository: ${REPO_URL}" +log_info "Branch: ${BRANCH}" + +# Validate required inputs +if [[ -z "${ARCANE_URL}" ]]; then + log_error "arcane-url is required" + exit 1 +fi +if [[ -z "${API_KEY}" ]]; then + log_error "arcane-api-key is required" + exit 1 +fi +if [[ -z "${ENV_ID}" ]]; then + log_error "environment-id is required" + exit 1 +fi +if [[ -z "${COMPOSE_DIR}" && -z "${COMPOSE_FILES_INPUT}" ]]; then + log_error "At least one of compose-dir or compose-files is required" + exit 1 +fi + +# Mask the API key +echo "::add-mask::${API_KEY}" + +# Export shared env vars +export_env_vars + +# Discover compose files +echo "::group::Discovering compose files" +compose_file_list=$(discover_compose_files) +mapfile -t compose_files <<< "${compose_file_list}" +log_info "Found ${#compose_files[@]} compose file(s):" +for f in "${compose_files[@]}"; do + log_info " - ${f}" +done +echo "::endgroup::" + +# Ensure git repository exists in Arcane +ensure_repository + +# Get existing syncs +echo "::group::Syncing compose stacks" +existing_syncs=$(arcane_api GET "/environments/${ENV_ID}/gitops-syncs") + +# Process each compose file +for compose_path in "${compose_files[@]}"; do + [[ -z "${compose_path}" ]] && continue + + sync_name=$(sync_name_from_path "${compose_path}") + log_info "Processing: ${compose_path} -> ${sync_name}" + upsert_sync "${compose_path}" "${sync_name}" "${existing_syncs}" +done +echo "::endgroup::" + +# Set outputs +echo "syncs-created=${SYNCS_CREATED}" >> "${GITHUB_OUTPUT}" +echo "syncs-updated=${SYNCS_UPDATED}" >> "${GITHUB_OUTPUT}" +echo "repository-id=${REPOSITORY_ID}" >> "${GITHUB_OUTPUT}" + +log_info "Done! Created: ${SYNCS_CREATED}, Updated: ${SYNCS_UPDATED}" diff --git a/.github/actions/arcane-deploy/action.yml b/.github/actions/arcane-deploy/action.yml new file mode 100644 index 0000000..a9a2869 --- /dev/null +++ b/.github/actions/arcane-deploy/action.yml @@ -0,0 +1,121 @@ +name: 'Arcane Docker Compose Deploy' +description: 'Deploy Docker Compose stacks to Arcane via GitOps sync. Discovers compose files and creates/updates syncs automatically.' +author: 'nsheaps' + +branding: + icon: 'upload-cloud' + color: 'purple' + +inputs: + arcane-url: + description: 'Base URL of the Arcane instance (e.g. https://arcane.example.com)' + required: true + + arcane-api-key: + description: 'API key for Arcane authentication (from Settings > API Keys)' + required: true + + environment-id: + description: 'Arcane environment ID to deploy to' + required: true + + # Compose file discovery + compose-dir: + description: 'Directory to scan for compose files (relative to repo root). Files matching compose.y[a]ml or docker-compose.y[a]ml are auto-discovered.' + required: false + default: '' + + compose-files: + description: 'Newline-separated list of compose file paths (relative to repo root). Use this for explicit control instead of directory scanning.' + required: false + default: '' + + # Git repository configuration + repository-url: + description: 'Git repository URL for Arcane to clone. Defaults to the current GitHub repository HTTPS URL.' + required: false + default: '' + + repository-name: + description: 'Name for the repository in Arcane. Defaults to the GitHub repository name.' + required: false + default: '' + + branch: + description: 'Branch to sync from. Defaults to the branch that triggered the workflow.' + required: false + default: '' + + auth-type: + description: 'Git authentication type: none, http, or ssh' + required: false + default: 'http' + + git-token: + description: 'Token for HTTP git authentication. Required when auth-type is http.' + required: false + default: '' + + # Sync behavior + auto-sync: + description: 'Enable auto-sync polling in Arcane so it periodically pulls changes' + required: false + default: 'true' + + sync-interval: + description: 'Minutes between auto-sync polls' + required: false + default: '5' + + trigger-sync: + description: 'Trigger an immediate sync after creating or updating each stack' + required: false + default: 'true' + + sync-name-prefix: + description: 'Prefix for sync names in Arcane. Defaults to the GitHub repository name.' + required: false + default: '' + + # Shared environment variables + env-vars: + description: 'Shared environment variables (KEY=VALUE per line) to export for the workflow. Use these in your compose files via variable interpolation or .env files in your repo.' + required: false + default: '' + +outputs: + syncs-created: + description: 'Number of new syncs created' + value: ${{ steps.deploy.outputs.syncs-created }} + + syncs-updated: + description: 'Number of existing syncs updated' + value: ${{ steps.deploy.outputs.syncs-updated }} + + repository-id: + description: 'Arcane git repository ID used' + value: ${{ steps.deploy.outputs.repository-id }} + +runs: + using: 'composite' + steps: + - name: Deploy to Arcane + id: deploy + shell: bash + run: ${{ github.action_path }}/action.sh + env: + INPUT_ARCANE_URL: ${{ inputs.arcane-url }} + INPUT_ARCANE_API_KEY: ${{ inputs.arcane-api-key }} + INPUT_ENVIRONMENT_ID: ${{ inputs.environment-id }} + INPUT_COMPOSE_DIR: ${{ inputs.compose-dir }} + INPUT_COMPOSE_FILES: ${{ inputs.compose-files }} + INPUT_REPOSITORY_URL: ${{ inputs.repository-url }} + INPUT_REPOSITORY_NAME: ${{ inputs.repository-name }} + INPUT_BRANCH: ${{ inputs.branch }} + INPUT_AUTH_TYPE: ${{ inputs.auth-type }} + INPUT_GIT_TOKEN: ${{ inputs.git-token }} + INPUT_AUTO_SYNC: ${{ inputs.auto-sync }} + INPUT_SYNC_INTERVAL: ${{ inputs.sync-interval }} + INPUT_TRIGGER_SYNC: ${{ inputs.trigger-sync }} + INPUT_SYNC_NAME_PREFIX: ${{ inputs.sync-name-prefix }} + INPUT_ENV_VARS: ${{ inputs.env-vars }} diff --git a/README.md b/README.md index b789928..73e735e 100644 --- a/README.md +++ b/README.md @@ -89,6 +89,93 @@ Read a prompt template file and interpolate environment variables using envsubst run: echo "${{ steps.prompt.outputs.prompt }}" ``` +### Deployment Actions + +#### `arcane-deploy` + +Deploy Docker Compose stacks to [Arcane](https://github.com/getarcaneapp/arcane) via GitOps sync. Automatically discovers compose files and creates or updates syncs in Arcane — no manual git sync setup required. + +**Features:** + +- Auto-discover compose files from a directory +- Explicit compose file list for full control +- Creates the git repository in Arcane if it doesn't exist +- Creates or updates gitops syncs (never deletes) +- Shared environment variables across stacks +- Triggers immediate sync after changes + +**Directory scan** — automatically finds compose files in a directory: + +```yaml +- name: Deploy stacks to Arcane + uses: nsheaps/github-actions/.github/actions/arcane-deploy@main + with: + arcane-url: ${{ secrets.ARCANE_URL }} + arcane-api-key: ${{ secrets.ARCANE_API_KEY }} + environment-id: '1' + compose-dir: stacks + git-token: ${{ secrets.REPO_TOKEN }} +``` + +**Explicit file list** — deploy specific compose files: + +```yaml +- name: Deploy stacks to Arcane + uses: nsheaps/github-actions/.github/actions/arcane-deploy@main + with: + arcane-url: ${{ secrets.ARCANE_URL }} + arcane-api-key: ${{ secrets.ARCANE_API_KEY }} + environment-id: '1' + compose-files: | + services/web/compose.yml + services/api/compose.yml + services/db/compose.yml + git-token: ${{ secrets.REPO_TOKEN }} +``` + +**With shared environment variables:** + +```yaml +- name: Deploy stacks to Arcane + uses: nsheaps/github-actions/.github/actions/arcane-deploy@main + with: + arcane-url: ${{ secrets.ARCANE_URL }} + arcane-api-key: ${{ secrets.ARCANE_API_KEY }} + environment-id: '1' + compose-dir: stacks + git-token: ${{ secrets.REPO_TOKEN }} + env-vars: | + DOMAIN=example.com + NETWORK=traefik + TZ=America/New_York +``` + +**Inputs:** + +| Input | Required | Default | Description | +| ------------------ | -------- | --------------------- | ------------------------------------------------------- | +| `arcane-url` | Yes | | Base URL of the Arcane instance | +| `arcane-api-key` | Yes | | API key (from Arcane Settings > API Keys) | +| `environment-id` | Yes | | Arcane environment ID | +| `compose-dir` | No | | Directory to scan for compose files | +| `compose-files` | No | | Newline-separated list of compose file paths | +| `repository-url` | No | GitHub repo HTTPS URL | Git URL for Arcane to clone | +| `repository-name` | No | GitHub repo name | Display name in Arcane | +| `branch` | No | Triggering branch | Branch to sync from | +| `auth-type` | No | `http` | Git auth type: `none`, `http`, or `ssh` | +| `git-token` | No | | Token for HTTP git authentication | +| `auto-sync` | No | `true` | Enable Arcane auto-sync polling | +| `sync-interval` | No | `5` | Minutes between auto-sync polls | +| `trigger-sync` | No | `true` | Trigger immediate sync after create/update | +| `sync-name-prefix` | No | GitHub repo name | Prefix for sync names in Arcane | +| `env-vars` | No | | Shared env vars (`KEY=VALUE` per line) for the workflow | + +**Outputs:** + +- `syncs-created` — Number of new syncs created +- `syncs-updated` — Number of existing syncs updated +- `repository-id` — Arcane git repository ID used + ### Security Linter Actions All security linters are designed to run in parallel for comprehensive security scanning. From 99bae8b333f0c37a603b9069031f610bf5dc4ed2 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 23 Feb 2026 02:29:53 +0000 Subject: [PATCH 02/14] Move arcane-deploy detailed docs to action README The root README now has a brief entry matching the style of other actions, with a link to the full docs in .github/actions/arcane-deploy/README.md. https://claude.ai/code/session_01PrmzWtCEcxJTMjxXje3ndd --- .github/actions/arcane-deploy/README.md | 97 +++++++++++++++++++++++++ README.md | 72 +----------------- 2 files changed, 98 insertions(+), 71 deletions(-) create mode 100644 .github/actions/arcane-deploy/README.md diff --git a/.github/actions/arcane-deploy/README.md b/.github/actions/arcane-deploy/README.md new file mode 100644 index 0000000..2a2ae20 --- /dev/null +++ b/.github/actions/arcane-deploy/README.md @@ -0,0 +1,97 @@ +# Arcane Docker Compose Deploy + +Deploy Docker Compose stacks to [Arcane](https://github.com/getarcaneapp/arcane) via GitOps sync. Automatically discovers compose files and creates or updates syncs in Arcane — no manual git sync setup required. + +## Features + +- Auto-discover compose files from a directory +- Explicit compose file list for full control +- Creates the git repository in Arcane if it doesn't exist +- Creates or updates gitops syncs (never deletes) +- Shared environment variables across stacks +- Triggers immediate sync after changes + +## Usage + +**Directory scan** — automatically finds compose files in a directory: + +```yaml +- name: Deploy stacks to Arcane + uses: nsheaps/github-actions/.github/actions/arcane-deploy@main + with: + arcane-url: ${{ secrets.ARCANE_URL }} + arcane-api-key: ${{ secrets.ARCANE_API_KEY }} + environment-id: '1' + compose-dir: stacks + git-token: ${{ secrets.REPO_TOKEN }} +``` + +**Explicit file list** — deploy specific compose files: + +```yaml +- name: Deploy stacks to Arcane + uses: nsheaps/github-actions/.github/actions/arcane-deploy@main + with: + arcane-url: ${{ secrets.ARCANE_URL }} + arcane-api-key: ${{ secrets.ARCANE_API_KEY }} + environment-id: '1' + compose-files: | + services/web/compose.yml + services/api/compose.yml + services/db/compose.yml + git-token: ${{ secrets.REPO_TOKEN }} +``` + +**With shared environment variables:** + +```yaml +- name: Deploy stacks to Arcane + uses: nsheaps/github-actions/.github/actions/arcane-deploy@main + with: + arcane-url: ${{ secrets.ARCANE_URL }} + arcane-api-key: ${{ secrets.ARCANE_API_KEY }} + environment-id: '1' + compose-dir: stacks + git-token: ${{ secrets.REPO_TOKEN }} + env-vars: | + DOMAIN=example.com + NETWORK=traefik + TZ=America/New_York +``` + +## Inputs + +| Input | Required | Default | Description | +| ------------------ | -------- | --------------------- | ------------------------------------------------------- | +| `arcane-url` | Yes | | Base URL of the Arcane instance | +| `arcane-api-key` | Yes | | API key (from Arcane Settings > API Keys) | +| `environment-id` | Yes | | Arcane environment ID | +| `compose-dir` | No | | Directory to scan for compose files | +| `compose-files` | No | | Newline-separated list of compose file paths | +| `repository-url` | No | GitHub repo HTTPS URL | Git URL for Arcane to clone | +| `repository-name` | No | GitHub repo name | Display name in Arcane | +| `branch` | No | Triggering branch | Branch to sync from | +| `auth-type` | No | `http` | Git auth type: `none`, `http`, or `ssh` | +| `git-token` | No | | Token for HTTP git authentication | +| `auto-sync` | No | `true` | Enable Arcane auto-sync polling | +| `sync-interval` | No | `5` | Minutes between auto-sync polls | +| `trigger-sync` | No | `true` | Trigger immediate sync after create/update | +| `sync-name-prefix` | No | GitHub repo name | Prefix for sync names in Arcane | +| `env-vars` | No | | Shared env vars (`KEY=VALUE` per line) for the workflow | + +## Outputs + +| Output | Description | +| --------------- | --------------------------------- | +| `syncs-created` | Number of new syncs created | +| `syncs-updated` | Number of existing syncs updated | +| `repository-id` | Arcane git repository ID used | + +## How It Works + +1. **Discovers** compose files from `compose-dir` (scanning for `compose.y[a]ml` / `docker-compose.y[a]ml`) and/or the explicit `compose-files` list +2. **Ensures** a git repository exists in Arcane matching your repo URL (creates one if needed, updates credentials if it already exists) +3. **Upserts** a gitops sync for each compose file — matched by compose path + repository ID, so re-runs are idempotent +4. **Triggers** an immediate sync on each stack (unless `trigger-sync` is `false`) + +Sync names are derived from the compose file's parent directory: `stacks/myapp/compose.yml` becomes `-myapp`. diff --git a/README.md b/README.md index 73e735e..a68d2e6 100644 --- a/README.md +++ b/README.md @@ -93,47 +93,7 @@ Read a prompt template file and interpolate environment variables using envsubst #### `arcane-deploy` -Deploy Docker Compose stacks to [Arcane](https://github.com/getarcaneapp/arcane) via GitOps sync. Automatically discovers compose files and creates or updates syncs in Arcane — no manual git sync setup required. - -**Features:** - -- Auto-discover compose files from a directory -- Explicit compose file list for full control -- Creates the git repository in Arcane if it doesn't exist -- Creates or updates gitops syncs (never deletes) -- Shared environment variables across stacks -- Triggers immediate sync after changes - -**Directory scan** — automatically finds compose files in a directory: - -```yaml -- name: Deploy stacks to Arcane - uses: nsheaps/github-actions/.github/actions/arcane-deploy@main - with: - arcane-url: ${{ secrets.ARCANE_URL }} - arcane-api-key: ${{ secrets.ARCANE_API_KEY }} - environment-id: '1' - compose-dir: stacks - git-token: ${{ secrets.REPO_TOKEN }} -``` - -**Explicit file list** — deploy specific compose files: - -```yaml -- name: Deploy stacks to Arcane - uses: nsheaps/github-actions/.github/actions/arcane-deploy@main - with: - arcane-url: ${{ secrets.ARCANE_URL }} - arcane-api-key: ${{ secrets.ARCANE_API_KEY }} - environment-id: '1' - compose-files: | - services/web/compose.yml - services/api/compose.yml - services/db/compose.yml - git-token: ${{ secrets.REPO_TOKEN }} -``` - -**With shared environment variables:** +Deploy Docker Compose stacks to [Arcane](https://github.com/getarcaneapp/arcane) via GitOps sync. Auto-discovers compose files and creates/updates syncs. See [action README](.github/actions/arcane-deploy/README.md) for full docs. ```yaml - name: Deploy stacks to Arcane @@ -144,38 +104,8 @@ Deploy Docker Compose stacks to [Arcane](https://github.com/getarcaneapp/arcane) environment-id: '1' compose-dir: stacks git-token: ${{ secrets.REPO_TOKEN }} - env-vars: | - DOMAIN=example.com - NETWORK=traefik - TZ=America/New_York ``` -**Inputs:** - -| Input | Required | Default | Description | -| ------------------ | -------- | --------------------- | ------------------------------------------------------- | -| `arcane-url` | Yes | | Base URL of the Arcane instance | -| `arcane-api-key` | Yes | | API key (from Arcane Settings > API Keys) | -| `environment-id` | Yes | | Arcane environment ID | -| `compose-dir` | No | | Directory to scan for compose files | -| `compose-files` | No | | Newline-separated list of compose file paths | -| `repository-url` | No | GitHub repo HTTPS URL | Git URL for Arcane to clone | -| `repository-name` | No | GitHub repo name | Display name in Arcane | -| `branch` | No | Triggering branch | Branch to sync from | -| `auth-type` | No | `http` | Git auth type: `none`, `http`, or `ssh` | -| `git-token` | No | | Token for HTTP git authentication | -| `auto-sync` | No | `true` | Enable Arcane auto-sync polling | -| `sync-interval` | No | `5` | Minutes between auto-sync polls | -| `trigger-sync` | No | `true` | Trigger immediate sync after create/update | -| `sync-name-prefix` | No | GitHub repo name | Prefix for sync names in Arcane | -| `env-vars` | No | | Shared env vars (`KEY=VALUE` per line) for the workflow | - -**Outputs:** - -- `syncs-created` — Number of new syncs created -- `syncs-updated` — Number of existing syncs updated -- `repository-id` — Arcane git repository ID used - ### Security Linter Actions All security linters are designed to run in parallel for comprehensive security scanning. From ae9d19382449747ac51d646a84d3c7a31c1cc787 Mon Sep 17 00:00:00 2001 From: nsheaps <1282393+nsheaps@users.noreply.github.com> Date: Mon, 23 Feb 2026 02:30:17 +0000 Subject: [PATCH 03/14] chore: `mise format` Triggered by: 420104a6ab29ad9752754b7043039b384840ee53 Workflow run: https://github.com/nsheaps/github-actions/actions/runs/22290862379 --- .github/actions/arcane-deploy/README.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/actions/arcane-deploy/README.md b/.github/actions/arcane-deploy/README.md index 2a2ae20..14b02ab 100644 --- a/.github/actions/arcane-deploy/README.md +++ b/.github/actions/arcane-deploy/README.md @@ -81,11 +81,11 @@ Deploy Docker Compose stacks to [Arcane](https://github.com/getarcaneapp/arcane) ## Outputs -| Output | Description | -| --------------- | --------------------------------- | -| `syncs-created` | Number of new syncs created | -| `syncs-updated` | Number of existing syncs updated | -| `repository-id` | Arcane git repository ID used | +| Output | Description | +| --------------- | -------------------------------- | +| `syncs-created` | Number of new syncs created | +| `syncs-updated` | Number of existing syncs updated | +| `repository-id` | Arcane git repository ID used | ## How It Works From 68cab3d4322171b3015ead369c98ba2f294d2160 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 23 Feb 2026 19:19:02 +0000 Subject: [PATCH 04/14] Add PR review reports (in progress) Review agents for simplicity, flexibility, security, and usability have completed. Remaining categories (documentation, patterns, best-practices, quality-assurance) are still running and will be added in a follow-up commit. https://claude.ai/code/session_01PrmzWtCEcxJTMjxXje3ndd --- .../1/1771874092/flexibility/REPORT.md | 86 +++++++++++++++ .../1/1771874092/security/REPORT.md | 78 ++++++++++++++ .../1/1771874092/simplicity/REPORT.md | 81 ++++++++++++++ .../1/1771874092/usability/REPORT.md | 100 ++++++++++++++++++ 4 files changed, 345 insertions(+) create mode 100644 .claude/pr-reviews/nsheaps/github-actions/1/1771874092/flexibility/REPORT.md create mode 100644 .claude/pr-reviews/nsheaps/github-actions/1/1771874092/security/REPORT.md create mode 100644 .claude/pr-reviews/nsheaps/github-actions/1/1771874092/simplicity/REPORT.md create mode 100644 .claude/pr-reviews/nsheaps/github-actions/1/1771874092/usability/REPORT.md diff --git a/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/flexibility/REPORT.md b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/flexibility/REPORT.md new file mode 100644 index 0000000..a825ed6 --- /dev/null +++ b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/flexibility/REPORT.md @@ -0,0 +1,86 @@ +# Flexibility Review + +**Score: 62/100** + +The action provides a solid foundation for the most common use case -- deploying a mono-repo of Docker Compose stacks to a single Arcane environment via HTTP-authenticated git -- and it exposes a good set of inputs with sensible defaults. However, several design choices introduce hard limits that will frustrate users with more complex or non-standard setups: the `maxdepth 2` directory scan, the sync naming scheme that collapses nested paths into a single directory name, the lack of support for SSH key material, the inability to target multiple environments, and the absence of a dry-run or plan mode. The action covers the 80% case well but falls short on the remaining 20% of real-world scenarios. + +## Detailed Findings + +### Strengths + +**1. Dual compose file discovery (good)** +Users can choose between directory scanning (`compose-dir`) and an explicit list (`compose-files`), and these two modes can be combined (action.sh:71-97). This is a well-designed flexibility point -- auto-discovery for convention-based repos, explicit lists for everything else. + +**2. Sensible defaults derived from GitHub context (good)** +Branch defaults to `GITHUB_REF_NAME`, repository URL defaults to `https://github.com/${GITHUB_REPOSITORY}.git`, sync name prefix defaults to the repo name (action.sh:10-19). Users deploying from a standard GitHub Actions workflow need to provide only the three required inputs plus at least one compose source to get started. + +**3. Every meaningful knob is exposed as an input (good)** +Auto-sync, sync interval, trigger-sync, auth type, sync-name-prefix, env-vars -- the action does not hard-code behavioral choices (action.yml:59-84). Users who want to disable auto-sync polling or change the interval can do so. + +**4. Idempotent upsert matched by composePath + repositoryId (good)** +The sync matching logic (action.sh:186-189) uses compose path and repository ID rather than name, which means renames of the sync-name-prefix do not create duplicates. This is a pragmatic design choice. + +**5. Outputs for downstream integration (good)** +The action exports `syncs-created`, `syncs-updated`, and `repository-id` (action.yml:86-97, action.sh:340-342), allowing subsequent steps to make conditional decisions (e.g., post a Slack notification if syncs-created > 0). + +### Weaknesses + +**6. `maxdepth 2` is too restrictive and not configurable (significant)** +The `find` command at action.sh:91 uses `-maxdepth 2`. This means only compose files at `/` and `//` are discovered. A repo structured as `stacks/team-a/service-foo/compose.yml` (depth 3) will be silently missed. There is no input to override the max depth. Users must fall back to the explicit `compose-files` list to work around this, which defeats the convenience of auto-discovery. + +**7. Sync naming collapses nested paths, risking collisions (significant)** +`sync_name_from_path` (action.sh:111-127) extracts only `basename(dirname(path))`. Given two compose files `frontend/web/compose.yml` and `backend/web/compose.yml`, both would produce the sync name `-web`, causing the second to overwrite the first during upsert. There is no input to provide a custom naming function or template. The only workaround is to use `compose-files` and ensure directory names are globally unique, which is a constraint the action imposes rather than documenting. + +**8. SSH auth type is declared but not actually supported (moderate)** +`auth-type` accepts `ssh` (action.yml:49-52), but the action never handles SSH key material. There is no `ssh-key` input. The `create_payload` at action.sh:160-165 passes `authType: "ssh"` with an empty token, which will likely fail on the Arcane API side or produce a non-functional repository. Users who need SSH-based cloning (e.g., for private repos behind a bastion or deploy key setup) have no path forward without modifying the action. + +**9. No support for multiple environments in a single invocation (moderate)** +The action takes a single `environment-id`. Organizations that deploy the same stacks to staging and production in a single workflow must call the action twice, duplicating the compose discovery and repository-ensure steps. A list of environment IDs (or a matrix-friendly design that outputs the discovered files for reuse) would be more flexible. + +**10. env-vars are exported to GITHUB_ENV, not to Arcane (moderate)** +The `env-vars` input (action.sh:260-278) writes to `$GITHUB_ENV`, which means the variables are available to subsequent steps in the GitHub workflow but are NOT sent to Arcane as stack-level environment variables. The README (README.md:56-59) shows `DOMAIN=example.com` as if it will be available inside compose, but those variables only exist in the runner process. If Arcane does not interpolate compose files at clone time using the repo's `.env` file, these variables will have no effect on the deployed stacks. This is a flexibility gap and potentially misleading documentation. + +**11. No dry-run or plan mode (moderate)** +There is no way to preview what the action would do without actually creating or updating syncs. A `dry-run: true` input that logs the planned API calls without executing them would make the action much safer to adopt and debug in CI pipelines. + +**12. No way to remove stale syncs (minor-to-moderate)** +The action creates and updates syncs but never deletes them (action README.md:10 explicitly states "never deletes"). If a compose file is removed from the repo, the corresponding Arcane sync becomes orphaned. There is no `prune: true` flag or lifecycle management. While "never delete" is safe, it pushes cleanup onto the user with no tooling support. + +**13. Compose file discovery does not deduplicate (minor)** +If a user provides both `compose-dir: stacks` and `compose-files: stacks/myapp/compose.yml`, the same file appears twice in the list. The upsert logic will process it twice, with the second call being a no-op update, but it wastes an API call and produces confusing log output. + +**14. Repository URL matching is exact-string, not normalized (minor)** +`ensure_repository` (action.sh:138-140) matches repositories by exact URL string comparison. `https://github.com/org/repo.git` and `https://github.com/org/repo` (without `.git`) would be treated as different repositories, potentially creating duplicates in Arcane. + +**15. No support for custom compose file name patterns (minor)** +The `find` command (action.sh:91-96) only matches `docker-compose.yml`, `docker-compose.yaml`, `compose.yml`, and `compose.yaml`. Users who name their files differently (e.g., `docker-compose.prod.yml`, `compose.override.yml`) have no way to extend the pattern without using the explicit file list. + +**16. sync-interval has no validation (minor)** +The `sync-interval` input is passed through as-is. Non-numeric values or values below a minimum threshold (if Arcane enforces one) will produce opaque API errors rather than a clear validation message. + +### Edge Cases and Scenarios Not Handled + +- **Monorepo with 50+ services at depth 3+**: Must use explicit file list; auto-discovery will not work. +- **Two teams with identically named services**: Sync name collision with no override mechanism. +- **Self-hosted Arcane behind VPN with SSH-only git**: `auth-type: ssh` is accepted but non-functional. +- **Deploying same stacks to dev/staging/prod**: Requires three separate action invocations with full input duplication. +- **Removing a service from the repo**: Orphaned sync in Arcane with no automated cleanup. +- **Compose files with non-standard names** (`docker-compose.prod.yml`): Not discoverable via directory scan. +- **Repository URL with or without `.git` suffix**: May create duplicate repositories in Arcane. + +## References + +- Action shell script: `/home/user/github-actions/.github/actions/arcane-deploy/action.sh` + - Compose file discovery: lines 67-105 + - Sync naming logic: lines 111-127 + - `maxdepth 2` limitation: line 91 + - Repository URL matching: lines 138-140 + - Env var export to GITHUB_ENV: lines 260-278 + - Outputs: lines 340-342 +- Action metadata: `/home/user/github-actions/.github/actions/arcane-deploy/action.yml` + - Input definitions: lines 9-84 + - Output definitions: lines 86-97 + - Composite step execution: lines 99-121 +- Action README: `/home/user/github-actions/.github/actions/arcane-deploy/README.md` + - "Never deletes" statement: line 10 + - Sync naming documentation: line 97 diff --git a/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/security/REPORT.md b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/security/REPORT.md new file mode 100644 index 0000000..0090745 --- /dev/null +++ b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/security/REPORT.md @@ -0,0 +1,78 @@ +# Security Review + +**Score: 52/100** + +This action has several meaningful security concerns that should be addressed before merging. While some good practices are present (use of `set -euo pipefail`, `jq --arg` for safe JSON construction, temp file cleanup), the script has critical gaps: the git token is never masked, the `env-vars` input is vulnerable to `GITHUB_ENV` injection, there is no URL scheme validation on `arcane-url` (SSRF risk), and multiple user-controlled inputs flow into shell contexts without sanitization. The action also passes secrets through the `env` block in `action.yml` without masking them before the script runs, meaning framework-level log exposure is possible before `::add-mask::` is called in the script body. + +## Detailed Findings + +### Critical Issues + +**C1. Git token (`git-token`) is never masked** (`action.sh` lines 14, 146-154, 159-165) + +The API key is masked on line 307 with `::add-mask::${API_KEY}`, but the git token -- which is equally sensitive -- is never masked. It is sent in JSON payloads to the Arcane API and could appear in error output (line 57: `cat "${tmp_body}" >&2` dumps the full API response body on failure, which could echo back the token). The `claude-auth` reference action masks its secret immediately. The git token should be masked with `::add-mask::${GIT_TOKEN}` right alongside the API key mask on line 307. + +**C2. `GITHUB_ENV` injection via `env-vars` input** (`action.sh` lines 260-278) + +The `export_env_vars` function writes user-supplied `KEY=VALUE` lines directly to `$GITHUB_ENV` (line 275). A malicious or misconfigured input can exploit the multiline GITHUB_ENV delimiter syntax to inject arbitrary environment variables into subsequent workflow steps. For example, an `env-vars` value like: + +``` +BENIGN=value +GITHUB_TOKEN<&2` dumps the entire response body. API error responses sometimes echo back request headers or body content, which could include the API key (sent in `X-Api-Key` header) or the git token (sent in JSON payloads). This output will appear in workflow logs. The response body should be sanitized or truncated before logging. + +**W3. Path traversal / injection via `compose-dir` and `compose-files`** (`action.sh` lines 8-9, 82, 91) + +The `compose-dir` input is concatenated into a `find` command target path (line 91) and the `compose-files` values are used as-is. While `find` itself is not vulnerable to injection here (it's not passed through `eval`), a path like `../../etc` could cause the action to scan outside the repository workspace. Similarly, the compose file paths are sent to the Arcane API, and a crafted path could potentially cause Arcane to read unexpected files from the repository. Consider validating that resolved paths stay within `$GITHUB_WORKSPACE`. + +**W4. No input sanitization for values written to `GITHUB_OUTPUT`** (`action.sh` lines 340-342) + +The values for `syncs-created`, `syncs-updated`, and `repository-id` are written to `$GITHUB_OUTPUT`. While `syncs-created` and `syncs-updated` are controlled integers, `repository-id` comes from an API response parsed through `jq`. If the API were compromised, a crafted `id` value containing newlines could inject additional output parameters. Using the heredoc delimiter pattern for GITHUB_OUTPUT would be safer. + +**W5. `sync-interval` is not validated as a number** (`action.sh` line 16, line 203) + +`SYNC_INTERVAL` comes from user input and is passed to `jq` with `--argjson` (line 203). If a non-numeric value is supplied, `jq` will fail, but a crafted JSON literal (e.g., `{"__proto__":1}`) could potentially be injected since `--argjson` parses its value as JSON. The input should be validated as a positive integer before use. + +**W6. `curl` does not enforce HTTPS** (`action.sh` line 47) + +The `curl` calls do not use `--proto '=https'` to enforce HTTPS-only connections. While the URL is user-configured, adding `--proto '=https'` (or at least validating the URL scheme) would prevent accidental or malicious use of plaintext HTTP, which would expose the API key in transit. The `claude-auth` action uses `--proto "=https"` in its Doppler curl call (claude-auth `action.sh` line 75), setting a precedent in this repository. + +### Good Practices + +**G1. `set -euo pipefail` is used** (`action.sh` line 2) -- This ensures the script fails fast on errors, unset variables, and pipe failures. This is stricter than the `claude-auth` action which only uses `set -e`. + +**G2. `jq --arg` is used for JSON construction** (`action.sh` lines 139, 148-150, 160-165, 187-189, 198-203, 225-231) -- All user-controlled values are passed to `jq` via `--arg`, which properly escapes them as JSON strings. This prevents JSON injection. This is a strong security pattern. + +**G3. API key is masked (even if late)** (`action.sh` line 307) -- The `::add-mask::` workflow command is used for the API key, which prevents it from appearing in subsequent log output. + +**G4. Temp files are cleaned up** (`action.sh` lines 58, 63) -- The `arcane_api` function creates a temp file and removes it in both the error and success paths. + +**G5. `find -print0` with `read -d ''` for safe filename handling** (`action.sh` lines 88-96) -- Null-delimited output from `find` is correctly consumed, preventing issues with spaces or special characters in file paths. + +**G6. No use of `eval` or unsafe shell expansion** -- The script avoids `eval`, backtick command substitution, and unquoted variable expansions in dangerous positions. Variables are consistently double-quoted. + +## References + +- `/home/user/github-actions/.github/actions/arcane-deploy/action.sh` -- Main deployment script (all line references above) +- `/home/user/github-actions/.github/actions/arcane-deploy/action.yml` -- Composite action definition, env block at lines 106-121 +- `/home/user/github-actions/.github/actions/claude-auth/action.sh` -- Reference action for security pattern comparison (masking at line 179, `--proto "=https"` at line 75) +- GitHub Security Advisory: [Untrusted input flowing into GITHUB_ENV](https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#using-an-intermediate-environment-variable) +- GitHub Docs: [Understanding the risk of script injections](https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#understanding-the-risk-of-script-injections) diff --git a/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/simplicity/REPORT.md b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/simplicity/REPORT.md new file mode 100644 index 0000000..467f541 --- /dev/null +++ b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/simplicity/REPORT.md @@ -0,0 +1,81 @@ +# Simplicity Review + +**Score: 42/100** + +The arcane-deploy action is a moderately over-engineered bash script that tries to be a full-featured API client for the Arcane platform rather than a focused, minimal deployment step. At 345 lines of bash and 15 inputs, it carries significant cognitive overhead for what could be a simpler interaction. The script contains a custom API client abstraction, compose file discovery with two different strategies (directory scanning and explicit listing), an environment variable export mechanism, a sync naming derivation scheme, and a full repository upsert flow. While each individual piece is readable, the aggregate complexity is high for a composite GitHub Action, and several features feel like they belong in the Arcane platform itself or in a dedicated CLI tool rather than in a bash script passed through environment variables. + +## Detailed Findings + +### Input Surface Area is Large (15 inputs) + +The action defines 15 inputs in `action.yml` (lines 9-84). Compare this to the existing actions in the repo: `claude-auth` has 12 inputs but supports three entirely different secret providers (Doppler, 1Password, raw), and `interpolate-prompt` has 1 input. For a single deployment target (Arcane), 15 inputs is a lot. Many of these have sensible defaults derived from GitHub context, which is good, but the sheer count creates a wide surface area that users need to understand. + +Inputs that add questionable value: +- **`sync-name-prefix`** (`action.yml:75-78`): A naming customization that could simply default to the repo name with no override needed for the common case. +- **`sync-interval`** (`action.yml:66-68`): Exposing polling interval at the action level is a deployment concern that most users will never change from the default of 5 minutes. +- **`env-vars`** (`action.yml:80-84`): This input writes KEY=VALUE pairs into `$GITHUB_ENV` (see `action.sh:260-278`). This is a workflow-level concern, not a deployment concern. Users can already set environment variables using the native `env:` key in their workflow YAML. Bundling this into the action conflates responsibilities. +- **`repository-name`** (`action.yml:39-42`) and **`repository-url`** (`action.yml:35-37`): These already default to the GitHub context values and are rarely needed. They add clutter. + +### Dual Compose File Discovery is an Unnecessary Abstraction + +The script supports two modes of discovering compose files: directory scanning (`compose-dir`, `action.sh:80-97`) and explicit listing (`compose-files`, `action.sh:71-78`). Supporting both is more complex than necessary. The directory scanning uses `find` with `-maxdepth 2` and matches four different filename patterns (`action.sh:91-96`). The explicit list requires newline-separated input with manual whitespace trimming (`action.sh:72-76`). + +A simpler approach: just accept an explicit list of compose file paths. If users want directory scanning, they can do that in a preceding workflow step and pass the result. This would cut out the `discover_compose_files` function entirely (lines 67-105) and simplify the required validation at lines 301-304. + +### The `env-vars` Feature Does Not Belong Here + +The `export_env_vars` function (`action.sh:260-278`) parses a multiline string of KEY=VALUE pairs and writes them to `$GITHUB_ENV`. This is orthogonal to the deployment logic. It also includes its own comment-stripping logic (`action.sh:270`) and whitespace trimming (`action.sh:267-268`). Users can achieve the same result natively in GitHub Actions: + +```yaml +env: + DOMAIN: example.com + NETWORK: traefik +``` + +This feature adds complexity without adding capability that does not already exist in the platform. + +### The API Helper is Reasonable but Uses Temp Files Unnecessarily + +The `arcane_api` function (`action.sh:37-64`) is a clean abstraction for making authenticated curl requests. However, it uses `mktemp` to capture the body (`action.sh:44`), then `cat`s the result (`action.sh:62`), then `rm`s the file (`action.sh:63`). This temp-file dance could be replaced with process substitution or a simpler variable capture. On the positive side, the separation of HTTP status code from body is a useful pattern, and the error handling at lines 55-60 is clear. + +### Sync Naming Logic is Fragile + +The `sync_name_from_path` function (`action.sh:111-127`) derives a sync name from a file path using `dirname` and `basename`. The special case where the directory name already matches the prefix (`action.sh:121-123`) is a subtle behavior that would surprise users. The naming scheme is documented in `README.md:97`, but the implementation has an implicit collision risk: two compose files in sibling directories with the same name (e.g., `a/myapp/compose.yml` and `b/myapp/compose.yml`) would produce the same sync name. + +### The Script is Linear but Long + +At 345 lines, `action.sh` is the longest script among the actions in this repo. The `claude-auth/action.sh` is 196 lines but handles three completely different authentication providers. The arcane-deploy script handles one provider (Arcane) but does more things: discovery, repository management, sync upsert, env var export. Each function is reasonably clear in isolation, but the total is a lot to follow. The main flow (`action.sh:280-344`) is well-structured and reads top-to-bottom, which is a positive. + +### Positive: Follows Repo Patterns Well + +The action follows the established pattern of the repository: composite action with `action.yml` + `action.sh`, `INPUT_*` env vars passed from the YAML to the script, `set -euo pipefail`, logging helpers, and GitHub-native annotations (`::error::`, `::group::`, `::add-mask::`). This consistency with `claude-auth` and other actions is good for maintainability. + +### Positive: Idempotent Upsert Pattern + +The upsert logic in `upsert_sync` (`action.sh:179-257`) is well-designed. It matches existing syncs by compose path + repository ID (`action.sh:186-189`), which is a stable identity key. This means re-running the action does not create duplicates. The repository upsert in `ensure_repository` (`action.sh:131-175`) similarly checks by URL before creating. This is a good practice. + +### Positive: Good README + +The action README (`README.md`) is well-structured with clear usage examples for the three main modes (directory scan, explicit list, env vars). The inputs table at lines 64-80 and the "How It Works" section at lines 92-97 give users a quick orientation. The root `README.md` entry at lines 94-107 is appropriately brief and links to the detailed docs. + +### Comparison Summary + +| Metric | `arcane-deploy` | `claude-auth` | `interpolate-prompt` | `lint-trivy` | +|--------|-----------------|---------------|----------------------|--------------| +| Inputs | 15 | 12 | 1 | 0 | +| Script lines | 345 | 196 | (inline, 18) | (inline, 8) | +| Functions | 7 | 5 | 0 | 0 | +| External deps | curl, jq, find | curl, doppler, op | envsubst | trivy | + +The arcane-deploy action is roughly 2x the complexity of claude-auth while solving a narrower problem (one platform vs. three providers). This ratio suggests there is room for simplification. + +## References + +- `/home/user/github-actions/.github/actions/arcane-deploy/action.sh` - Main deployment script (345 lines) +- `/home/user/github-actions/.github/actions/arcane-deploy/action.yml` - Action definition with 15 inputs (122 lines) +- `/home/user/github-actions/.github/actions/arcane-deploy/README.md` - Action documentation (98 lines) +- `/home/user/github-actions/README.md` - Root README with arcane-deploy entry at lines 94-107 +- `/home/user/github-actions/.github/actions/claude-auth/action.yml` - Comparable action for pattern reference (96 lines, 12 inputs) +- `/home/user/github-actions/.github/actions/claude-auth/action.sh` - Comparable script for pattern reference (196 lines) +- `/home/user/github-actions/.github/actions/interpolate-prompt/action.yml` - Simple action for contrast (43 lines, 1 input) +- `/home/user/github-actions/.github/actions/lint-trivy/action.yml` - Minimal action for contrast (25 lines, 0 inputs) diff --git a/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/usability/REPORT.md b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/usability/REPORT.md new file mode 100644 index 0000000..18625b1 --- /dev/null +++ b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/usability/REPORT.md @@ -0,0 +1,100 @@ +# Usability Review + +**Score: 72/100** + +This action delivers a solid first-time user experience with clear documentation, sensible defaults, and well-structured log output using GitHub Actions grouping. However, several usability gaps reduce confidence during failure scenarios: the API key is masked after it may already have been logged, `git-token` is never masked at all, error messages from the Arcane API are dumped as raw JSON without interpretation, the `env-vars` feature has confusing semantics that could mislead users, and there is no validation for several inputs where invalid values would cause cryptic downstream failures. A new user can get a basic deployment running quickly, but when something goes wrong, debugging will require significant Arcane API knowledge that the action does not surface. + +## Detailed Findings + +### Secret Masking (Critical) + +1. **API key masked too late.** At `action.sh:307`, `::add-mask::` is called for the API key, but lines 283-286 have already logged `ARCANE_URL`, `ENV_ID`, and other configuration values. While the API key itself is not logged in those lines, the mask call occurs after the script has already begun executing and emitting output. If any error or debug trace were to include the key before line 307, it would be exposed. Best practice is to mask secrets as the very first operation. + + - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, line 307 + +2. **`git-token` is never masked.** The `git-token` input is a secret credential used for repository authentication, but `::add-mask::` is never called for it. If any error message, API response body, or debug output includes the token value, it will be visible in plain text in the workflow logs. This is a significant security/usability concern since users would reasonably expect all secret inputs to be protected. + + - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, lines 14, 149-150, 165 + +3. **`env-vars` values are never masked.** Environment variable values set via `env-vars` may contain secrets (database passwords, API keys, etc.), but only the key names are logged (line 274 shows `KEY=***`). However, the values are written to `GITHUB_ENV` without masking, so downstream steps could inadvertently log them. The action should call `::add-mask::` on each value. + + - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, lines 260-278 + +### Error Messages and Debugging + +4. **API error bodies are dumped as raw JSON.** When an API call fails (HTTP >= 400), line 57 runs `cat "${tmp_body}" >&2`, which dumps the raw Arcane API response to stderr. This could be an opaque JSON blob that means nothing to a user unfamiliar with the Arcane API. A more usable approach would be to extract a human-readable message (e.g., using `jq` to pull a `.message` or `.error` field) and provide guidance on common errors (401 = bad API key, 404 = invalid environment ID, etc.). + + - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, lines 55-59 + +5. **No validation of `auth-type` values.** The `auth-type` input accepts any string, but only `none`, `http`, and `ssh` are valid. If a user typos this as `https` or `token`, the action will silently pass the invalid value to the Arcane API and fail with an unhelpful API error. + + - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, line 13; `action.yml`, line 49-51 + +6. **No validation of `sync-interval` as a number.** The `sync-interval` input is passed directly to `--argjson` in `jq` (lines 203, 237). If a user provides a non-numeric value like `"five"`, `jq` will fail with a cryptic error about invalid JSON rather than a clear message like "sync-interval must be a number." + + - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, lines 203, 237 + +7. **No warning when `auth-type` is `http` but `git-token` is empty.** Lines 146-155 only update credentials if `auth-type` is `http` AND `git-token` is non-empty, but there is no warning emitted if a user sets `auth-type: http` (the default) without providing a `git-token`. This will likely cause Arcane to fail when cloning the repository, with no indication from this action that the configuration is incomplete. + + - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, lines 146, 163-165; `action.yml`, line 55-57 + +8. **`jq` dependency is assumed but not checked.** The script relies heavily on `jq` (lines 138, 148, 160, 170, 186, 198, 225, 247) but never verifies that `jq` is available. If run on a runner image without `jq`, the error will be a shell "command not found" error, which is not actionable. A pre-flight check or a note in the README about dependencies would improve usability. + + - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, multiple lines + +### Logging and GitHub Actions Groups + +9. **Good use of `::group::` sections.** The action uses four well-scoped groups: "Discovering compose files" (lines 313-320), "Ensuring git repository in Arcane" (lines 132-174), "Syncing compose stacks" (lines 326-337), and "Setting shared environment variables" (lines 265-277). These collapse nicely in the GitHub Actions UI and make it easy to expand only the relevant section during debugging. This is well done. + +10. **Initial configuration log is helpful.** Lines 282-286 log the Arcane URL, environment ID, repository URL, and branch at the top of the run, giving immediate visibility into what the action will do. This is good practice. + +11. **Sync trigger failures are silently swallowed.** Lines 219 and 254 use `|| true` after triggering a sync, which means a failed sync trigger produces no output at all. The user would see "Triggering sync..." but never know whether it succeeded or failed. At minimum, the action should log whether the trigger succeeded. + + - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, lines 219, 254 + +### Documentation and Getting Started + +12. **README provides three clear usage examples.** The action README (`README.md`) shows directory scan, explicit file list, and environment variables patterns. Each example is complete and copy-pasteable. The inputs table is comprehensive and well-formatted. This makes onboarding straightforward. + + - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/README.md`, lines 16-60 + +13. **Root README provides appropriate summary.** The root `README.md` includes the action under a "Deployment Actions" section with a concise description and a minimal example, then links to the detailed README. This two-level documentation approach is good. + + - **File:** `/home/user/github-actions/README.md`, lines 93-107 + +14. **`action.yml` input descriptions are clear and contextual.** The descriptions include examples (e.g., `e.g. https://arcane.example.com` for `arcane-url`, `from Settings > API Keys` for the API key) and explain defaults in natural language. The comments grouping inputs by category (`# Compose file discovery`, `# Git repository configuration`, etc.) aid readability even though they do not appear in the GitHub UI. + + - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.yml`, lines 9-84 + +15. **Missing: what to do when things go wrong.** Neither the action README nor the root README includes a troubleshooting section. Common failure modes (invalid API key, wrong environment ID, repository clone failures, compose file not found at the expected path) should be documented with suggested fixes. + +16. **Missing: prerequisites section.** The README does not mention that users need an Arcane instance, an API key (and how to create one beyond "Settings > API Keys"), or what permissions are required. A brief "Prerequisites" section would help new users who are not yet Arcane administrators. + +### `env-vars` Feature Semantics + +17. **Confusing `env-vars` behavior.** The `env-vars` input writes variables to `GITHUB_ENV` (line 275), which makes them available to subsequent workflow steps but has no effect on the Arcane deployment itself. The `action.yml` description says "Use these in your compose files via variable interpolation or .env files in your repo," but environment variables exported to `GITHUB_ENV` are not sent to Arcane and are not available inside compose files on the Arcane host. This description is misleading and could cause confusion. A user might expect these variables to be injected into the remote Docker Compose environment. + + - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, line 275; `action.yml`, lines 81-84 + +### Outputs + +18. **Outputs are useful but minimal.** The three outputs (`syncs-created`, `syncs-updated`, `repository-id`) enable basic downstream logic (e.g., conditional steps based on whether anything changed). However, the action does not output sync IDs, sync names, or a list of compose files processed, which limits more advanced workflows. For example, a user wanting to add a deployment status comment to a PR would need the individual sync names/IDs. + + - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.yml`, lines 86-97 + +### Minor Issues + +19. **`set -euo pipefail` combined with `|| true` patterns.** The script uses `set -euo pipefail` (line 2) but then uses `|| true` in several places (lines 53, 219, 254). While this is technically correct, the `|| true` on the curl command at line 53 means that network errors (DNS failures, timeouts) will not produce a non-zero HTTP code and will instead silently produce an empty response body that fails in `jq` later with a confusing error. + + - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, line 53 + +20. **Compose file discovery does not deduplicate.** If a user provides both `compose-dir` and `compose-files` with overlapping paths, the same file will be processed twice, creating duplicate syncs. There is no deduplication logic. + + - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, lines 67-105 + +## References + +- Action script: `/home/user/github-actions/.github/actions/arcane-deploy/action.sh` +- Action metadata: `/home/user/github-actions/.github/actions/arcane-deploy/action.yml` +- Action README: `/home/user/github-actions/.github/actions/arcane-deploy/README.md` +- Root README: `/home/user/github-actions/README.md` From 097f141e0b8c2243ae9a4948599c658717cafd1f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 23 Feb 2026 19:19:48 +0000 Subject: [PATCH 05/14] Add remaining review reports (documentation, best-practices, patterns pending) https://claude.ai/code/session_01PrmzWtCEcxJTMjxXje3ndd --- .../1/1771874092/best-practices/REPORT.md | 101 ++++++++++++++ .../1/1771874092/documentation/REPORT.md | 123 ++++++++++++++++++ .../1/1771874092/patterns/REPORT.md | 107 +++++++++++++++ 3 files changed, 331 insertions(+) create mode 100644 .claude/pr-reviews/nsheaps/github-actions/1/1771874092/best-practices/REPORT.md create mode 100644 .claude/pr-reviews/nsheaps/github-actions/1/1771874092/documentation/REPORT.md create mode 100644 .claude/pr-reviews/nsheaps/github-actions/1/1771874092/patterns/REPORT.md diff --git a/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/best-practices/REPORT.md b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/best-practices/REPORT.md new file mode 100644 index 0000000..8adc20e --- /dev/null +++ b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/best-practices/REPORT.md @@ -0,0 +1,101 @@ +# Best Practices Review + +**Score: 86/100** + +This PR demonstrates strong adherence to best practices across shell scripting, GitHub Actions design, and API integration. The code is well-structured with clear separation of concerns, consistent error handling, idempotent API operations, and proper use of GitHub Actions workflow commands. The main areas for improvement involve a few defensive coding gaps in the shell script, a missing trap-based cleanup pattern for temp files, and a second commit that does not follow conventional commit format. + +## Detailed Findings + +### Excellent Practices + +**Shell Scripting (Strong)** +- `set -euo pipefail` is present on line 2 of `action.sh`, ensuring fail-fast behavior for unset variables, non-zero exits, and pipeline failures. +- Variables are consistently and correctly double-quoted throughout the entire script (e.g., `"${ARCANE_URL}"`, `"${compose_path}"`), preventing word splitting and globbing issues. +- `local` variable declarations are used in every function (`local method`, `local path`, `local files=()`, `local http_code`, etc.), preventing variable leakage into the global scope. +- `jq` is used consistently for all JSON construction and parsing (lines 138, 148, 160, 186, 198, 225, 247), avoiding fragile string interpolation for JSON payloads. The use of `--arg` and `--argjson` is correct and avoids injection risks. +- The `find` command uses `-print0` with `read -r -d ''` (lines 88-96), correctly handling filenames with spaces or special characters. +- Whitespace trimming uses pure bash parameter expansion (lines 73-74, 267-268), avoiding unnecessary subprocess calls. + +**GitHub Actions Structure (Strong)** +- The composite action in `action.yml` is cleanly organized with descriptive input names, good descriptions, sensible defaults, and clear groupings via comments. +- Branding is properly configured with `icon: 'upload-cloud'` and `color: 'purple'` (lines 5-7 of `action.yml`). +- Outputs are properly wired from the step ID to the action outputs (lines 86-97 of `action.yml`). +- The step has a clear `name: Deploy to Arcane` and an explicit `id: deploy` for output references. +- Environment variables are passed explicitly from inputs to the shell script via the `env:` block (lines 107-121 of `action.yml`), which is the correct pattern for composite actions rather than relying on `${{ inputs.* }}` in shell code. + +**GitHub Actions Workflow Commands (Strong)** +- `::error::` is used in `log_error()` (line 31), causing errors to appear as annotations in the Actions UI. +- `::group::` / `::endgroup::` is used for logical sections: "Ensuring git repository" (line 132/174), "Discovering compose files" (line 313/320), "Syncing compose stacks" (line 326/337), and "Setting shared environment variables" (line 265/277). +- `::add-mask::` is used for the API key (line 307), preventing it from leaking in logs. + +**API Integration (Strong)** +- The `arcane_api()` helper (lines 37-64) centralizes all HTTP requests with consistent authentication, error handling, and HTTP status code checking (`http_code -ge 400`). +- The upsert pattern in `upsert_sync()` (lines 179-257) matches by `composePath` + `repositoryId`, making the operation fully idempotent on re-runs. +- `ensure_repository()` (lines 131-175) follows the same find-or-create pattern, and also updates credentials on existing repos to keep tokens current. +- Error responses include both the HTTP status code and the response body (lines 56-58), which aids debugging. + +**Documentation (Strong)** +- The `README.md` provides usage examples for all three patterns (directory scan, explicit list, environment variables), a complete inputs/outputs table, and a clear "How It Works" explanation. +- The root `README.md` is updated with a concise entry and a link to the detailed docs. + +**Input Validation (Good)** +- Required inputs are validated early (lines 289-304) with descriptive error messages and `exit 1`. +- The mutual requirement of `compose-dir` or `compose-files` is explicitly checked (lines 301-304). + +### Areas for Improvement + +**1. Temp File Cleanup via Trap (Medium Impact)** +In `arcane_api()` (lines 43-63), a temp file is created with `mktemp` and cleaned up manually with `rm -f`. If the function is interrupted between `mktemp` and `rm -f` (e.g., by a signal or `set -e` triggering on `cat`), the temp file will be leaked. Best practice is to use a `trap` for cleanup: +```bash +local tmp_body +tmp_body=$(mktemp) +trap "rm -f '${tmp_body}'" RETURN +``` +Alternatively, a global trap at the top of the script could manage a temp directory. + +**2. Second Commit Does Not Follow Conventional Commits (Low-Medium Impact)** +The first commit (`feat: add arcane-deploy action for Docker Compose GitOps sync`) follows conventional commit format correctly. The second commit (`Move arcane-deploy detailed docs to action README`) does not use a conventional prefix. It should be something like `docs: move arcane-deploy detailed docs to action README`. While not a blocking issue, consistency with conventional commits is a stated best practice for this repository given the first commit's format. + +**3. Missing `::add-mask::` for `GIT_TOKEN` (Medium Impact)** +The API key is masked on line 307 with `::add-mask::`, but `GIT_TOKEN` (which contains a git authentication credential) is never masked. If it appears in any log output (e.g., in an error response from the API), it would be visible in plain text. A mask should be added: +```bash +if [[ -n "${GIT_TOKEN}" ]]; then + echo "::add-mask::${GIT_TOKEN}" +fi +``` + +**4. `curl` Failure Suppressed with `|| true` (Low-Medium Impact)** +On line 53, `|| true` suppresses curl's own exit code (e.g., DNS resolution failure, connection refused, timeout). This means a network-level failure would result in an empty `http_code` rather than a caught error. The subsequent `[[ "${http_code}" -ge 400 ]]` comparison would fail unpredictably on an empty string. A more robust approach: +```bash +http_code=$(curl -s -w "%{http_code}" ... -o "${tmp_body}" "$@" "${url}") || { + log_error "API ${method} ${path} - curl failed (network error)" + rm -f "${tmp_body}" + return 1 +} +``` + +**5. No Retry Logic for Transient API Failures (Low Impact)** +API calls are single-shot with no retry. For a deployment action, transient 5xx errors or network blips could cause unnecessary failures. While not strictly required, a simple retry with backoff (even just 1-2 retries) would improve reliability. This is a nice-to-have rather than a requirement. + +**6. `export_env_vars` Uses `local` Outside a Function Context Concern (Low Impact)** +In `export_env_vars()` on line 272, `local key` and `local value` are declared inside a `while` loop that is itself inside a function, which is fine. However, the values written to `GITHUB_ENV` (line 275) are not masked. If sensitive values are passed via `env-vars`, they could appear in logs. The function logs `${key}=***` which is good, but consider also masking the values: +```bash +echo "::add-mask::${value}" +``` + +**7. Composite Action Has a Single Step (Informational)** +The action runs all logic in a single composite step. While this is perfectly functional, some best practice guides suggest splitting validation, discovery, and API operations into separate steps for better observability in the Actions UI. The use of `::group::` within the script mitigates this well, but separate steps would give individual step-level timing and status indicators. + +**8. `action.sh` Line Count (Informational)** +At 345 lines, the script is at the upper bound of comfortable single-file shell script size. If more features are added, consider splitting into helper modules sourced from the main script. + +## References + +- `/home/user/github-actions/.github/actions/arcane-deploy/action.sh` - Main shell script (345 lines) +- `/home/user/github-actions/.github/actions/arcane-deploy/action.yml` - Composite action definition (122 lines) +- `/home/user/github-actions/.github/actions/arcane-deploy/README.md` - Action documentation (97 lines) +- `/home/user/github-actions/README.md` - Root repository README (179 lines) +- [GitHub Actions - Creating a composite action](https://docs.github.com/en/actions/sharing-automations/creating-actions/creating-a-composite-action) +- [GitHub Actions - Workflow commands](https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions) +- [Conventional Commits specification](https://www.conventionalcommits.org/en/v1.0.0/) +- [Google Shell Style Guide](https://google.github.io/styleguide/shellguide.html) diff --git a/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/documentation/REPORT.md b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/documentation/REPORT.md new file mode 100644 index 0000000..899cad3 --- /dev/null +++ b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/documentation/REPORT.md @@ -0,0 +1,123 @@ +# Documentation & Comments Review + +**Score: 82/100** + +This PR delivers strong, well-structured documentation that significantly exceeds the baseline set by existing actions in the repository. The action README is comprehensive, covering inputs, outputs, usage examples, and a "How It Works" section. Inline code comments in the shell script are appropriate and well-organized. The main gaps are the absence of troubleshooting guidance, prerequisite/dependency documentation, and a few places where the "How It Works" section could more accurately reflect edge-case behavior in the script. + +## Detailed Findings + +### Action README (`.github/actions/arcane-deploy/README.md`) -- Excellent + +**Strengths:** + +- The README is the only action-specific README in the entire repository. No other action under `.github/actions/` has its own README, making `arcane-deploy` the best-documented action by a wide margin. This sets a positive precedent. +- The Features section (lines 6-13) gives a quick, scannable summary of capabilities. +- Three distinct usage examples (lines 16-60) cover the most common patterns: directory scan, explicit file list, and shared environment variables. Each is realistic and uses proper secrets references. +- The Inputs table (lines 62-80) is complete -- all 14 inputs from `action.yml` are represented with accurate descriptions and defaults. +- The Outputs table (lines 82-89) matches the three outputs defined in `action.yml`. +- The "How It Works" section (lines 90-97) explains the four-phase pipeline clearly: discover, ensure repository, upsert syncs, trigger sync. + +**Gaps and issues:** + +1. **Missing prerequisite documentation.** The script depends on `curl`, `jq`, and `find` being available on the runner. While these are standard on `ubuntu-latest`, the README does not mention runner requirements. A one-line note like "Requires a Linux runner with `curl` and `jq` installed" would help users on custom or minimal runners. + +2. **No troubleshooting section.** Common failure modes (invalid API key, unreachable Arcane URL, no compose files found, `compose-dir` not existing) produce specific error messages in the script but are not documented. A brief troubleshooting or "Common Errors" section would improve self-service debugging. + +3. **No mention of the "never deletes" policy beyond the Features bullet.** The Features section states "Creates or updates gitops syncs (never deletes)" (line 11), which is an important operational detail. However, the "How It Works" section does not reinforce this, nor does it explain what happens to syncs that were previously created but whose compose files have since been removed. This is a meaningful edge case for users managing evolving stacks. + +4. **Sync naming edge cases not fully documented.** The README says (line 97): `stacks/myapp/compose.yml` becomes `-myapp`. But the script at lines 117-124 has additional logic: if the directory basename already equals the prefix, it does not double-prefix (the `name != SYNC_NAME_PREFIX` check). And if the file is at the root (`dir == "."`), the sync name is just the prefix. The root-case is not documented at all, and the deduplication logic is not mentioned. + +5. **`maxdepth 2` limitation not documented.** The script's `find` at line 91 uses `-maxdepth 2`, meaning deeply nested compose files will not be discovered. The README says "Directory to scan for compose files" but does not specify that scanning only goes two levels deep. + +6. **Missing information about what `env-vars` actually does.** The README says env vars are "for the workflow" and the input description says "Use these in your compose files via variable interpolation or .env files in your repo." But the script (lines 260-278) exports them to `$GITHUB_ENV`, making them available to subsequent steps in the workflow -- it does not directly inject them into the Arcane environment or compose files. This distinction could confuse users. + +### action.yml (`.github/actions/arcane-deploy/action.yml`) -- Very Good + +**Strengths:** + +- Input descriptions (lines 9-84) are clear and actionable. The `arcane-api-key` description helpfully includes "(from Settings > API Keys)" guiding users to where to find the value. +- Inputs are logically grouped with YAML comments: compose file discovery (line 22), git repository configuration (line 33), sync behavior (line 59), shared environment variables (line 80). This is a pattern not used by other actions in the repo (`github-app-auth`, `claude-debug`, `claude-auth`) and improves scanability. +- The `compose-dir` description (line 24) includes the glob pattern hint `compose.y[a]ml or docker-compose.y[a]ml`, matching exactly what the script searches for. +- Output descriptions (lines 86-97) are concise and clear. +- Branding is set (lines 5-7), which is good practice and not done by all actions in the repo. + +**Gaps:** + +1. **No `author` field on comparable actions.** The `action.yml` includes `author: 'nsheaps'` (line 3), which only `claude-auth` among existing actions also does. This is a minor positive for attribution. + +2. **`auth-type` does not document valid values in enough detail.** Line 50 says `'Git authentication type: none, http, or ssh'` but does not explain what each implies (e.g., `http` requires `git-token`, `ssh` presumably requires keys configured elsewhere, `none` means public repo). The README's Inputs table similarly just says `none`, `http`, or `ssh` without elaboration. + +3. **`sync-interval` units.** The description says "Minutes between auto-sync polls" (line 66), which is good. However, it does not mention valid ranges or whether there is a minimum/maximum enforced by the Arcane API. + +### action.sh (`.github/actions/arcane-deploy/action.sh`) -- Good + +**Strengths:** + +- Well-structured section comments using `# --- Section Name ---` pattern (lines 4, 25, 34, 66, 107, 129, 177, 259, 280). This makes the 345-line script easy to navigate. +- The `arcane_api` function has a clear docstring: "Make an authenticated API request to Arcane." with a usage line (lines 35-36). +- The `sync_name_from_path` function includes concrete examples in its comment (lines 108-110): `"stacks/myapp/compose.yml" -> "${prefix}-myapp"` and `"compose.yml" (root) -> "${prefix}"`. This is excellent. +- The `ensure_repository` function has a one-line docstring (line 130): "Find an existing Arcane git repository by URL, or create one." +- The `upsert_sync` function has a one-line docstring (line 178): "Create or update a gitops sync for a single compose file." +- Inline comments at critical decision points: "Update credentials so the token stays current" (line 145), "Match by compose path + repository ID" (line 185), "Mask the API key" (line 306). +- Uses GitHub Actions log grouping (`::group::` / `::endgroup::`) for organized CI output (lines 132, 174, 265, 277, 313, 320, 326, 337). + +**Gaps:** + +1. **Whitespace trimming is uncommented.** Lines 73-74 and 267-268 use a bash parameter expansion pattern for trimming whitespace that is not immediately obvious: `file="${file#"${file%%[![:space:]]*}"}"`. While this is a known bash idiom, a brief comment like `# trim leading whitespace` (which does appear conceptually in the trailing-trim comment `# trim trailing`) would help. Actually, looking again, line 73 does have `# trim leading` and line 74 has `# trim trailing` -- these are present but placed as end-of-line comments that are easy to miss. This is acceptable. + +2. **`discover_compose_files` lacks a function-level docstring.** Unlike `arcane_api`, `sync_name_from_path`, `ensure_repository`, and `upsert_sync`, the `discover_compose_files` function (line 67) has only the section header `# --- Compose File Discovery ---` but no docstring explaining what it returns or its behavior when both `COMPOSE_FILES_INPUT` and `COMPOSE_DIR` are set (it processes both and concatenates results). + +3. **`export_env_vars` lacks a function-level docstring.** Line 260 similarly only has the section header. A brief note explaining that it writes to `$GITHUB_ENV` for subsequent steps would be helpful. + +4. **No comment explaining the `|| true` on curl.** Line 53 has `"${url}") || true` -- the `|| true` prevents the `set -e` from aborting when curl returns a non-zero exit code (which it does for certain network errors). This is a subtle but important pattern that warrants a brief comment. + +5. **No comment on why `sort -z` is used.** Line 96 uses `sort -z` for null-delimited sorting. While correct, a brief note on deterministic ordering would help readers unfamiliar with null-delimited pipelines. + +### Root README (`README.md`) -- Appropriate + +**Strengths:** + +- The `arcane-deploy` entry (lines 94-107) follows the established pattern of the other actions: a heading, one-line description, one usage example. +- It correctly links to the full docs: `See [action README](.github/actions/arcane-deploy/README.md) for full docs` (line 96). No other action in the root README links to a separate README (because none exist), making this a new pattern that is cleanly introduced. +- The entry is placed under a new "Deployment Actions" section (line 92), which is a logical categorization that keeps the root README organized as the action count grows. + +**Gaps:** + +1. **No outputs listed in the root README entry.** Other actions like `github-app-auth` (lines 21-26) list their outputs in the root README. The `arcane-deploy` entry omits outputs, relying on the linked README instead. This is a reasonable choice given the "See full docs" link, but is a slight inconsistency with the existing pattern. + +### Commit Messages -- Good + +- The first commit (`02dbdd6`) uses conventional commit format (`feat: add arcane-deploy action for Docker Compose GitOps sync`) with a descriptive body that covers the key capabilities. This is well-structured. +- The second commit (`99bae8b`) explains the doc restructuring clearly: "The root README now has a brief entry matching the style of other actions, with a link to the full docs." +- Both commits include the Claude session link, which is consistent with the repository's apparent convention. + +### Discoverability -- Very Good + +- A user browsing the root README would find the action under "Deployment Actions" with a link to full docs. +- A user browsing the `.github/actions/` directory would find the README alongside `action.yml` and `action.sh`. +- The `action.yml` descriptions are detailed enough to serve as inline documentation in workflow editors that display input descriptions. +- The only gap is the lack of any search-friendly keywords or tags. GitHub does not natively support action tags, but mentioning "Portainer" (which Arcane appears to be related to or forked from) or "Docker Compose GitOps" in the README would help users searching for solutions. + +### Summary of Deductions + +| Category | Deduction | Reason | +|----------|-----------|--------| +| Missing troubleshooting/error docs | -5 | No guidance on common failure modes | +| Missing prerequisites | -2 | No runner/dependency requirements noted | +| Incomplete edge-case docs | -4 | maxdepth 2 limit, root-path sync naming, never-deletes implications | +| env-vars behavior mismatch | -2 | README implies compose-level injection, script does GITHUB_ENV export | +| auth-type values underdocumented | -2 | No explanation of what each auth type requires | +| Missing function docstrings | -2 | `discover_compose_files` and `export_env_vars` lack docstrings | +| Minor inline comment gaps | -1 | `|| true` on curl, `sort -z` rationale | + +## References + +- `.github/actions/arcane-deploy/action.sh` -- Main deployment script (345 lines) +- `.github/actions/arcane-deploy/action.yml` -- Action definition with 14 inputs, 3 outputs +- `.github/actions/arcane-deploy/README.md` -- Comprehensive action documentation (98 lines) +- `README.md` -- Root repository README with arcane-deploy entry at lines 92-107 +- `.github/actions/github-app-auth/action.yml` -- Comparison action (no README, simpler descriptions) +- `.github/actions/claude-debug/action.yml` -- Comparison action (no README, inline script) +- `.github/actions/claude-auth/action.yml` -- Comparison action (no README, grouped inputs with comments) +- Commit `02dbdd6` -- feat: add arcane-deploy action for Docker Compose GitOps sync +- Commit `99bae8b` -- Move arcane-deploy detailed docs to action README diff --git a/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/patterns/REPORT.md b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/patterns/REPORT.md new file mode 100644 index 0000000..8005504 --- /dev/null +++ b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/patterns/REPORT.md @@ -0,0 +1,107 @@ +# Pattern Matching Review + +**Score: 62/100** + +The `arcane-deploy` action follows many of the repo's structural conventions -- composite action with `action.yml` + `action.sh`, kebab-case inputs, `INPUT_` env var mapping, and branding metadata -- but it deviates from the established shell scripting patterns in several notable ways. The existing `claude-auth/action.sh` is the closest comparable script (the only other standalone `.sh` file), and it establishes conventions for colored output, `set -e` (not `set -euo pipefail`), dedicated helper functions (`mask_value()`, `set_output()`, `export_env_var()`), and 4-space indentation. The new action diverges on all of these without comment or justification, which fragments the codebase's internal consistency. Additionally, the second commit message lacks a conventional-commit prefix, breaking the repo's commit convention. The action does introduce a well-crafted per-action `README.md` pattern that is a net positive but would benefit from adoption guidance. + +## Detailed Findings + +### Consistent Patterns + +1. **Composite action structure**: Uses `using: 'composite'` with a `steps` block and `shell: bash`, matching every other action in the repo (`claude-auth`, `github-app-auth`, `interpolate-prompt`, `claude-debug`, all `lint-*` actions). + +2. **File organization (`action.yml` + `action.sh`)**: The only other action that externalizes its script is `claude-auth`. The new action follows this same `action.yml` + `action.sh` separation pattern, which is appropriate given its 345-line script length. + +3. **`INPUT_` env var mapping**: Inputs are passed from `action.yml` to the shell script via `INPUT_*` environment variables (e.g., `INPUT_ARCANE_URL: ${{ inputs.arcane-url }}`). This matches the pattern in `claude-auth/action.yml` exactly. + +4. **Kebab-case input names**: All inputs use kebab-case (`arcane-url`, `compose-dir`, `sync-name-prefix`), consistent with every other action in the repo. + +5. **`branding` block**: Includes `icon` and `color`, matching `claude-auth`, `github-app-auth`, `interpolate-prompt`, and `claude-debug`. + +6. **`author` field**: Includes `author: 'nsheaps'`, matching `claude-auth`. + +7. **Output mechanism**: Uses `echo "key=value" >> "$GITHUB_OUTPUT"`, consistent with how `claude-auth`, `claude-debug`, `github-app-auth`, and `interpolate-prompt` set outputs. + +8. **`::add-mask::` usage**: Masks the API key on line 307 of `action.sh`, matching how `claude-auth` uses `mask_value()` (which wraps the same `::add-mask::` call). + +9. **`log_info()` and `log_error()` function naming**: Matches the function names used in `claude-auth/action.sh` (lines 11-17). + +10. **`::group::` / `::endgroup::` usage**: Used in `action.sh` lines 132, 174, 265, 277, 313, 320, 326, 337. This matches the usage in `claude-debug/action.yml` (line 81, 206). Good for log readability. + +### Pattern Deviations + +1. **`set -euo pipefail` vs `set -e`** (Severity: Medium) + - Existing: `claude-auth/action.sh` line 2 uses `set -e` + - New: `arcane-deploy/action.sh` line 2 uses `set -euo pipefail` + - Assessment: `set -euo pipefail` is arguably more robust (catches unset variables and pipe failures), but it introduces an inconsistency. If this is the desired direction, `claude-auth/action.sh` should be updated to match. Without that follow-up, it is an unjustified divergence. + +2. **No colored output** (Severity: Medium) + - Existing: `claude-auth/action.sh` defines `RED`, `GREEN`, `YELLOW`, `NC` color variables and uses `echo -e` with color codes throughout (lines 5-8, 12-21). + - New: `arcane-deploy/action.sh` uses plain `echo` with a `[Arcane Deploy]` prefix (lines 27-28) and `::error::` annotations (line 31). + - Assessment: The `::error::` annotation approach is actually better for GitHub Actions (it surfaces errors in the Actions UI), but the lack of colored `log_info` output is inconsistent. In practice, `echo -e` with ANSI colors does render in Actions logs, so there is visible user-facing inconsistency. + +3. **No `set_output()` / `mask_value()` / `export_env_var()` helper functions** (Severity: Medium) + - Existing: `claude-auth/action.sh` defines reusable helpers `mask_value()` (line 24), `set_output()` (line 30), `export_env_var()` (line 37), and `claude-debug` defines an inline `set_output()` (line 84). + - New: `arcane-deploy/action.sh` inlines `::add-mask::` (line 307), `>> "$GITHUB_OUTPUT"` (lines 340-342), and `>> "$GITHUB_ENV"` (line 275) directly. + - Assessment: This is a missed opportunity for consistency. If the repo had a shared library, these would be imported; absent that, at least defining the same helper functions would improve readability and make future refactoring easier. + +4. **Indentation: 2 spaces vs 4 spaces** (Severity: Low) + - Existing: `claude-auth/action.sh` uses 4-space indentation throughout (e.g., lines 12-13, 25-27, 31-33). + - New: `arcane-deploy/action.sh` uses 2-space indentation throughout. + - Assessment: The repo's `.editorconfig` or `.prettierrc` may have a preference. Without a `.editorconfig` rule for `.sh` files, this is a minor but noticeable inconsistency. The 2-space style is common in bash scripts, but consistency within a repo matters more than community convention. + +5. **`log_warning()` absent** (Severity: Low) + - Existing: `claude-auth/action.sh` defines `log_warning()` (line 19). + - New: `arcane-deploy/action.sh` only defines `log_info()` and `log_error()`, no `log_warning()`. + - Assessment: Minor, but if the action needed warnings it would need to add one later. A complete set of logging functions is better. + +6. **Error annotation style** (Severity: Low) + - Existing: `claude-auth/action.sh` logs errors to stderr via `echo -e ... >&2` (line 17). `interpolate-prompt/action.yml` uses `echo "::error::..."` (line 29). + - New: `arcane-deploy/action.sh` combines both: `echo "::error::[Arcane Deploy] $1"` (line 31), but `log_info` does not go through `::notice::`. + - Assessment: The `::error::` approach is actually more useful in GitHub Actions as it creates annotations. This is a positive deviation for `log_error`, but inconsistent with `claude-auth`. + +7. **Quoting style** (Severity: Low) + - Existing: `claude-auth/action.sh` uses double quotes loosely (e.g., `echo "$name=$value"` without braces). + - New: `arcane-deploy/action.sh` consistently uses `"${variable}"` brace-quoted form. + - Assessment: The new action's style is more defensive and arguably better practice. Minor inconsistency. + +### New Patterns Introduced + +1. **Per-action `README.md`** (Merit: High) + - This is the first action in the repo to have its own `README.md` file. None of the other 12 actions have one. + - The README is well-structured: Features, Usage examples, Inputs table, Outputs table, and a "How It Works" section. + - The root `README.md` links to it with "See [action README](.github/actions/arcane-deploy/README.md) for full docs." + - Assessment: This is a strong positive pattern. The `arcane-deploy` action has 15 inputs -- far more than any other action -- so a dedicated README makes sense. For simpler actions like `lint-checkov` (0 inputs) this would be overkill, but for complex actions like `claude-auth` (12 inputs) or `claude-debug` (5 inputs), this pattern would be beneficial. **Recommendation**: Adopt this pattern for actions with more than ~5 inputs, and document this convention. + +2. **Section comment headers** (Merit: Medium) + - The script uses `# --- Section Name ---` comment blocks (lines 4, 25, 35, 66, 107, 129, 177, 259, 280) to organize the script into logical sections. + - `claude-auth/action.sh` has no such structural comments. + - Assessment: Good practice for a 345-line script. Makes navigation easier. Worth adopting. + +3. **API helper abstraction** (Merit: High) + - The `arcane_api()` function (lines 37-64) is a well-designed HTTP client abstraction that handles method, path, auth headers, error checking, and temp file cleanup in one place. + - Assessment: This is good engineering. It avoids repeated curl boilerplate and centralizes error handling. Not directly reusable by other actions, but the pattern of abstracting external service calls is worth noting. + +4. **`::group::` for logical sections** (Merit: Medium) + - Uses `::group::` / `::endgroup::` to wrap logical phases: compose file discovery (line 313), repository setup (line 132), sync processing (line 326), env vars (line 265). + - `claude-debug` uses this once (line 81). + - Assessment: Good practice for actions with multi-phase output. Makes logs much more readable in the Actions UI. Worth adopting in `claude-auth`. + +### Commit History + +- **Commit 1**: `feat: add arcane-deploy action for Docker Compose GitOps sync` -- Follows conventional commit format (`feat:` prefix). Good. +- **Commit 2**: `Move arcane-deploy detailed docs to action README` -- **Does not** follow conventional commit format. Should be `docs: move arcane-deploy detailed docs to action README` or `refactor: move arcane-deploy detailed docs to action README`. This breaks the pattern established by prior commits (`chore: mise format`, `fix: use mise task scripts...`, `feat: add shared GitHub Actions...`). + +## References + +| File | Role | Key Lines | +|------|------|-----------| +| `/home/user/github-actions/.github/actions/arcane-deploy/action.sh` | New action script | L2 (`set -euo pipefail`), L5-19 (config), L26-32 (logging), L37-64 (API helper), L307 (mask), L340-342 (outputs) | +| `/home/user/github-actions/.github/actions/arcane-deploy/action.yml` | New action definition | L1-7 (metadata+branding), L9-84 (inputs), L86-97 (outputs), L99-121 (runs+env mapping) | +| `/home/user/github-actions/.github/actions/arcane-deploy/README.md` | New action docs | Only action with a dedicated README | +| `/home/user/github-actions/.github/actions/claude-auth/action.sh` | Existing comparable script | L2 (`set -e`), L5-8 (colors), L11-21 (logging with colors), L24-41 (helpers), 4-space indent | +| `/home/user/github-actions/.github/actions/claude-auth/action.yml` | Existing comparable action.yml | L1-6 (metadata+branding), L9-69 (inputs), L71-74 (outputs), L76-96 (runs+env mapping) | +| `/home/user/github-actions/.github/actions/claude-debug/action.yml` | Existing action with inline script | L79-222 (inline bash with `set_output()`, `::group::`) | +| `/home/user/github-actions/.github/actions/interpolate-prompt/action.yml` | Existing action | L29 (`::error::` usage) | +| `/home/user/github-actions/.github/actions/github-app-auth/action.yml` | Existing action (no shell script) | L1-2 (metadata without `author`), L7-8 (branding) | +| `/home/user/github-actions/README.md` | Root docs | L92-107 (arcane-deploy section with link to action README) | From 6f5d7819af3267c50e36f7d3248fd4684f235a85 Mon Sep 17 00:00:00 2001 From: nsheaps <1282393+nsheaps@users.noreply.github.com> Date: Mon, 23 Feb 2026 19:19:55 +0000 Subject: [PATCH 06/14] chore: `mise format` Triggered by: 166213accbab5309050954b06cb990f50661aa58 Workflow run: https://github.com/nsheaps/github-actions/actions/runs/22321183132 --- .../1/1771874092/simplicity/REPORT.md | 13 +++++++------ .../1/1771874092/usability/REPORT.md | 16 ---------------- 2 files changed, 7 insertions(+), 22 deletions(-) diff --git a/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/simplicity/REPORT.md b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/simplicity/REPORT.md index 467f541..3d1a85c 100644 --- a/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/simplicity/REPORT.md +++ b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/simplicity/REPORT.md @@ -11,6 +11,7 @@ The arcane-deploy action is a moderately over-engineered bash script that tries The action defines 15 inputs in `action.yml` (lines 9-84). Compare this to the existing actions in the repo: `claude-auth` has 12 inputs but supports three entirely different secret providers (Doppler, 1Password, raw), and `interpolate-prompt` has 1 input. For a single deployment target (Arcane), 15 inputs is a lot. Many of these have sensible defaults derived from GitHub context, which is good, but the sheer count creates a wide surface area that users need to understand. Inputs that add questionable value: + - **`sync-name-prefix`** (`action.yml:75-78`): A naming customization that could simply default to the repo name with no override needed for the common case. - **`sync-interval`** (`action.yml:66-68`): Exposing polling interval at the action level is a deployment concern that most users will never change from the default of 5 minutes. - **`env-vars`** (`action.yml:80-84`): This input writes KEY=VALUE pairs into `$GITHUB_ENV` (see `action.sh:260-278`). This is a workflow-level concern, not a deployment concern. Users can already set environment variables using the native `env:` key in their workflow YAML. Bundling this into the action conflates responsibilities. @@ -60,12 +61,12 @@ The action README (`README.md`) is well-structured with clear usage examples for ### Comparison Summary -| Metric | `arcane-deploy` | `claude-auth` | `interpolate-prompt` | `lint-trivy` | -|--------|-----------------|---------------|----------------------|--------------| -| Inputs | 15 | 12 | 1 | 0 | -| Script lines | 345 | 196 | (inline, 18) | (inline, 8) | -| Functions | 7 | 5 | 0 | 0 | -| External deps | curl, jq, find | curl, doppler, op | envsubst | trivy | +| Metric | `arcane-deploy` | `claude-auth` | `interpolate-prompt` | `lint-trivy` | +| ------------- | --------------- | ----------------- | -------------------- | ------------ | +| Inputs | 15 | 12 | 1 | 0 | +| Script lines | 345 | 196 | (inline, 18) | (inline, 8) | +| Functions | 7 | 5 | 0 | 0 | +| External deps | curl, jq, find | curl, doppler, op | envsubst | trivy | The arcane-deploy action is roughly 2x the complexity of claude-auth while solving a narrower problem (one platform vs. three providers). This ratio suggests there is room for simplification. diff --git a/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/usability/REPORT.md b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/usability/REPORT.md index 18625b1..354e3b0 100644 --- a/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/usability/REPORT.md +++ b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/usability/REPORT.md @@ -9,37 +9,29 @@ This action delivers a solid first-time user experience with clear documentation ### Secret Masking (Critical) 1. **API key masked too late.** At `action.sh:307`, `::add-mask::` is called for the API key, but lines 283-286 have already logged `ARCANE_URL`, `ENV_ID`, and other configuration values. While the API key itself is not logged in those lines, the mask call occurs after the script has already begun executing and emitting output. If any error or debug trace were to include the key before line 307, it would be exposed. Best practice is to mask secrets as the very first operation. - - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, line 307 2. **`git-token` is never masked.** The `git-token` input is a secret credential used for repository authentication, but `::add-mask::` is never called for it. If any error message, API response body, or debug output includes the token value, it will be visible in plain text in the workflow logs. This is a significant security/usability concern since users would reasonably expect all secret inputs to be protected. - - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, lines 14, 149-150, 165 3. **`env-vars` values are never masked.** Environment variable values set via `env-vars` may contain secrets (database passwords, API keys, etc.), but only the key names are logged (line 274 shows `KEY=***`). However, the values are written to `GITHUB_ENV` without masking, so downstream steps could inadvertently log them. The action should call `::add-mask::` on each value. - - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, lines 260-278 ### Error Messages and Debugging 4. **API error bodies are dumped as raw JSON.** When an API call fails (HTTP >= 400), line 57 runs `cat "${tmp_body}" >&2`, which dumps the raw Arcane API response to stderr. This could be an opaque JSON blob that means nothing to a user unfamiliar with the Arcane API. A more usable approach would be to extract a human-readable message (e.g., using `jq` to pull a `.message` or `.error` field) and provide guidance on common errors (401 = bad API key, 404 = invalid environment ID, etc.). - - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, lines 55-59 5. **No validation of `auth-type` values.** The `auth-type` input accepts any string, but only `none`, `http`, and `ssh` are valid. If a user typos this as `https` or `token`, the action will silently pass the invalid value to the Arcane API and fail with an unhelpful API error. - - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, line 13; `action.yml`, line 49-51 6. **No validation of `sync-interval` as a number.** The `sync-interval` input is passed directly to `--argjson` in `jq` (lines 203, 237). If a user provides a non-numeric value like `"five"`, `jq` will fail with a cryptic error about invalid JSON rather than a clear message like "sync-interval must be a number." - - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, lines 203, 237 7. **No warning when `auth-type` is `http` but `git-token` is empty.** Lines 146-155 only update credentials if `auth-type` is `http` AND `git-token` is non-empty, but there is no warning emitted if a user sets `auth-type: http` (the default) without providing a `git-token`. This will likely cause Arcane to fail when cloning the repository, with no indication from this action that the configuration is incomplete. - - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, lines 146, 163-165; `action.yml`, line 55-57 8. **`jq` dependency is assumed but not checked.** The script relies heavily on `jq` (lines 138, 148, 160, 170, 186, 198, 225, 247) but never verifies that `jq` is available. If run on a runner image without `jq`, the error will be a shell "command not found" error, which is not actionable. A pre-flight check or a note in the README about dependencies would improve usability. - - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, multiple lines ### Logging and GitHub Actions Groups @@ -49,21 +41,17 @@ This action delivers a solid first-time user experience with clear documentation 10. **Initial configuration log is helpful.** Lines 282-286 log the Arcane URL, environment ID, repository URL, and branch at the top of the run, giving immediate visibility into what the action will do. This is good practice. 11. **Sync trigger failures are silently swallowed.** Lines 219 and 254 use `|| true` after triggering a sync, which means a failed sync trigger produces no output at all. The user would see "Triggering sync..." but never know whether it succeeded or failed. At minimum, the action should log whether the trigger succeeded. - - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, lines 219, 254 ### Documentation and Getting Started 12. **README provides three clear usage examples.** The action README (`README.md`) shows directory scan, explicit file list, and environment variables patterns. Each example is complete and copy-pasteable. The inputs table is comprehensive and well-formatted. This makes onboarding straightforward. - - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/README.md`, lines 16-60 13. **Root README provides appropriate summary.** The root `README.md` includes the action under a "Deployment Actions" section with a concise description and a minimal example, then links to the detailed README. This two-level documentation approach is good. - - **File:** `/home/user/github-actions/README.md`, lines 93-107 14. **`action.yml` input descriptions are clear and contextual.** The descriptions include examples (e.g., `e.g. https://arcane.example.com` for `arcane-url`, `from Settings > API Keys` for the API key) and explain defaults in natural language. The comments grouping inputs by category (`# Compose file discovery`, `# Git repository configuration`, etc.) aid readability even though they do not appear in the GitHub UI. - - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.yml`, lines 9-84 15. **Missing: what to do when things go wrong.** Neither the action README nor the root README includes a troubleshooting section. Common failure modes (invalid API key, wrong environment ID, repository clone failures, compose file not found at the expected path) should be documented with suggested fixes. @@ -73,23 +61,19 @@ This action delivers a solid first-time user experience with clear documentation ### `env-vars` Feature Semantics 17. **Confusing `env-vars` behavior.** The `env-vars` input writes variables to `GITHUB_ENV` (line 275), which makes them available to subsequent workflow steps but has no effect on the Arcane deployment itself. The `action.yml` description says "Use these in your compose files via variable interpolation or .env files in your repo," but environment variables exported to `GITHUB_ENV` are not sent to Arcane and are not available inside compose files on the Arcane host. This description is misleading and could cause confusion. A user might expect these variables to be injected into the remote Docker Compose environment. - - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, line 275; `action.yml`, lines 81-84 ### Outputs 18. **Outputs are useful but minimal.** The three outputs (`syncs-created`, `syncs-updated`, `repository-id`) enable basic downstream logic (e.g., conditional steps based on whether anything changed). However, the action does not output sync IDs, sync names, or a list of compose files processed, which limits more advanced workflows. For example, a user wanting to add a deployment status comment to a PR would need the individual sync names/IDs. - - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.yml`, lines 86-97 ### Minor Issues 19. **`set -euo pipefail` combined with `|| true` patterns.** The script uses `set -euo pipefail` (line 2) but then uses `|| true` in several places (lines 53, 219, 254). While this is technically correct, the `|| true` on the curl command at line 53 means that network errors (DNS failures, timeouts) will not produce a non-zero HTTP code and will instead silently produce an empty response body that fails in `jq` later with a confusing error. - - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, line 53 20. **Compose file discovery does not deduplicate.** If a user provides both `compose-dir` and `compose-files` with overlapping paths, the same file will be processed twice, creating duplicate syncs. There is no deduplication logic. - - **File:** `/home/user/github-actions/.github/actions/arcane-deploy/action.sh`, lines 67-105 ## References From 3de58edfd629a75a107368a2dca09838aacffef6 Mon Sep 17 00:00:00 2001 From: "automation-nsheaps[bot]" <251779498+automation-nsheaps[bot]@users.noreply.github.com> Date: Mon, 23 Feb 2026 19:21:02 +0000 Subject: [PATCH 07/14] chore: `mise format` Triggered by: 877bd47da4932ac12362d5edafcebbeb171e1349 Workflow run: https://github.com/nsheaps/github-actions/actions/runs/22321202828 --- .../1/1771874092/best-practices/REPORT.md | 11 ++++++++++ .../1/1771874092/documentation/REPORT.md | 18 +++++++-------- .../1/1771874092/patterns/REPORT.md | 22 +++++++++---------- 3 files changed, 31 insertions(+), 20 deletions(-) diff --git a/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/best-practices/REPORT.md b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/best-practices/REPORT.md index 8adc20e..0af164a 100644 --- a/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/best-practices/REPORT.md +++ b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/best-practices/REPORT.md @@ -9,6 +9,7 @@ This PR demonstrates strong adherence to best practices across shell scripting, ### Excellent Practices **Shell Scripting (Strong)** + - `set -euo pipefail` is present on line 2 of `action.sh`, ensuring fail-fast behavior for unset variables, non-zero exits, and pipeline failures. - Variables are consistently and correctly double-quoted throughout the entire script (e.g., `"${ARCANE_URL}"`, `"${compose_path}"`), preventing word splitting and globbing issues. - `local` variable declarations are used in every function (`local method`, `local path`, `local files=()`, `local http_code`, etc.), preventing variable leakage into the global scope. @@ -17,6 +18,7 @@ This PR demonstrates strong adherence to best practices across shell scripting, - Whitespace trimming uses pure bash parameter expansion (lines 73-74, 267-268), avoiding unnecessary subprocess calls. **GitHub Actions Structure (Strong)** + - The composite action in `action.yml` is cleanly organized with descriptive input names, good descriptions, sensible defaults, and clear groupings via comments. - Branding is properly configured with `icon: 'upload-cloud'` and `color: 'purple'` (lines 5-7 of `action.yml`). - Outputs are properly wired from the step ID to the action outputs (lines 86-97 of `action.yml`). @@ -24,21 +26,25 @@ This PR demonstrates strong adherence to best practices across shell scripting, - Environment variables are passed explicitly from inputs to the shell script via the `env:` block (lines 107-121 of `action.yml`), which is the correct pattern for composite actions rather than relying on `${{ inputs.* }}` in shell code. **GitHub Actions Workflow Commands (Strong)** + - `::error::` is used in `log_error()` (line 31), causing errors to appear as annotations in the Actions UI. - `::group::` / `::endgroup::` is used for logical sections: "Ensuring git repository" (line 132/174), "Discovering compose files" (line 313/320), "Syncing compose stacks" (line 326/337), and "Setting shared environment variables" (line 265/277). - `::add-mask::` is used for the API key (line 307), preventing it from leaking in logs. **API Integration (Strong)** + - The `arcane_api()` helper (lines 37-64) centralizes all HTTP requests with consistent authentication, error handling, and HTTP status code checking (`http_code -ge 400`). - The upsert pattern in `upsert_sync()` (lines 179-257) matches by `composePath` + `repositoryId`, making the operation fully idempotent on re-runs. - `ensure_repository()` (lines 131-175) follows the same find-or-create pattern, and also updates credentials on existing repos to keep tokens current. - Error responses include both the HTTP status code and the response body (lines 56-58), which aids debugging. **Documentation (Strong)** + - The `README.md` provides usage examples for all three patterns (directory scan, explicit list, environment variables), a complete inputs/outputs table, and a clear "How It Works" explanation. - The root `README.md` is updated with a concise entry and a link to the detailed docs. **Input Validation (Good)** + - Required inputs are validated early (lines 289-304) with descriptive error messages and `exit 1`. - The mutual requirement of `compose-dir` or `compose-files` is explicitly checked (lines 301-304). @@ -46,11 +52,13 @@ This PR demonstrates strong adherence to best practices across shell scripting, **1. Temp File Cleanup via Trap (Medium Impact)** In `arcane_api()` (lines 43-63), a temp file is created with `mktemp` and cleaned up manually with `rm -f`. If the function is interrupted between `mktemp` and `rm -f` (e.g., by a signal or `set -e` triggering on `cat`), the temp file will be leaked. Best practice is to use a `trap` for cleanup: + ```bash local tmp_body tmp_body=$(mktemp) trap "rm -f '${tmp_body}'" RETURN ``` + Alternatively, a global trap at the top of the script could manage a temp directory. **2. Second Commit Does Not Follow Conventional Commits (Low-Medium Impact)** @@ -58,6 +66,7 @@ The first commit (`feat: add arcane-deploy action for Docker Compose GitOps sync **3. Missing `::add-mask::` for `GIT_TOKEN` (Medium Impact)** The API key is masked on line 307 with `::add-mask::`, but `GIT_TOKEN` (which contains a git authentication credential) is never masked. If it appears in any log output (e.g., in an error response from the API), it would be visible in plain text. A mask should be added: + ```bash if [[ -n "${GIT_TOKEN}" ]]; then echo "::add-mask::${GIT_TOKEN}" @@ -66,6 +75,7 @@ fi **4. `curl` Failure Suppressed with `|| true` (Low-Medium Impact)** On line 53, `|| true` suppresses curl's own exit code (e.g., DNS resolution failure, connection refused, timeout). This means a network-level failure would result in an empty `http_code` rather than a caught error. The subsequent `[[ "${http_code}" -ge 400 ]]` comparison would fail unpredictably on an empty string. A more robust approach: + ```bash http_code=$(curl -s -w "%{http_code}" ... -o "${tmp_body}" "$@" "${url}") || { log_error "API ${method} ${path} - curl failed (network error)" @@ -79,6 +89,7 @@ API calls are single-shot with no retry. For a deployment action, transient 5xx **6. `export_env_vars` Uses `local` Outside a Function Context Concern (Low Impact)** In `export_env_vars()` on line 272, `local key` and `local value` are declared inside a `while` loop that is itself inside a function, which is fine. However, the values written to `GITHUB_ENV` (line 275) are not masked. If sensitive values are passed via `env-vars`, they could appear in logs. The function logs `${key}=***` which is good, but consider also masking the values: + ```bash echo "::add-mask::${value}" ``` diff --git a/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/documentation/REPORT.md b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/documentation/REPORT.md index 899cad3..47964ad 100644 --- a/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/documentation/REPORT.md +++ b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/documentation/REPORT.md @@ -100,15 +100,15 @@ This PR delivers strong, well-structured documentation that significantly exceed ### Summary of Deductions -| Category | Deduction | Reason | -|----------|-----------|--------| -| Missing troubleshooting/error docs | -5 | No guidance on common failure modes | -| Missing prerequisites | -2 | No runner/dependency requirements noted | -| Incomplete edge-case docs | -4 | maxdepth 2 limit, root-path sync naming, never-deletes implications | -| env-vars behavior mismatch | -2 | README implies compose-level injection, script does GITHUB_ENV export | -| auth-type values underdocumented | -2 | No explanation of what each auth type requires | -| Missing function docstrings | -2 | `discover_compose_files` and `export_env_vars` lack docstrings | -| Minor inline comment gaps | -1 | `|| true` on curl, `sort -z` rationale | +| Category | Deduction | Reason | +| ---------------------------------- | --------- | --------------------------------------------------------------------- | --- | -------------------------------- | +| Missing troubleshooting/error docs | -5 | No guidance on common failure modes | +| Missing prerequisites | -2 | No runner/dependency requirements noted | +| Incomplete edge-case docs | -4 | maxdepth 2 limit, root-path sync naming, never-deletes implications | +| env-vars behavior mismatch | -2 | README implies compose-level injection, script does GITHUB_ENV export | +| auth-type values underdocumented | -2 | No explanation of what each auth type requires | +| Missing function docstrings | -2 | `discover_compose_files` and `export_env_vars` lack docstrings | +| Minor inline comment gaps | -1 | ` | | true`on curl,`sort -z` rationale | ## References diff --git a/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/patterns/REPORT.md b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/patterns/REPORT.md index 8005504..71007ba 100644 --- a/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/patterns/REPORT.md +++ b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/patterns/REPORT.md @@ -94,14 +94,14 @@ The `arcane-deploy` action follows many of the repo's structural conventions -- ## References -| File | Role | Key Lines | -|------|------|-----------| -| `/home/user/github-actions/.github/actions/arcane-deploy/action.sh` | New action script | L2 (`set -euo pipefail`), L5-19 (config), L26-32 (logging), L37-64 (API helper), L307 (mask), L340-342 (outputs) | -| `/home/user/github-actions/.github/actions/arcane-deploy/action.yml` | New action definition | L1-7 (metadata+branding), L9-84 (inputs), L86-97 (outputs), L99-121 (runs+env mapping) | -| `/home/user/github-actions/.github/actions/arcane-deploy/README.md` | New action docs | Only action with a dedicated README | -| `/home/user/github-actions/.github/actions/claude-auth/action.sh` | Existing comparable script | L2 (`set -e`), L5-8 (colors), L11-21 (logging with colors), L24-41 (helpers), 4-space indent | -| `/home/user/github-actions/.github/actions/claude-auth/action.yml` | Existing comparable action.yml | L1-6 (metadata+branding), L9-69 (inputs), L71-74 (outputs), L76-96 (runs+env mapping) | -| `/home/user/github-actions/.github/actions/claude-debug/action.yml` | Existing action with inline script | L79-222 (inline bash with `set_output()`, `::group::`) | -| `/home/user/github-actions/.github/actions/interpolate-prompt/action.yml` | Existing action | L29 (`::error::` usage) | -| `/home/user/github-actions/.github/actions/github-app-auth/action.yml` | Existing action (no shell script) | L1-2 (metadata without `author`), L7-8 (branding) | -| `/home/user/github-actions/README.md` | Root docs | L92-107 (arcane-deploy section with link to action README) | +| File | Role | Key Lines | +| ------------------------------------------------------------------------- | ---------------------------------- | ---------------------------------------------------------------------------------------------------------------- | +| `/home/user/github-actions/.github/actions/arcane-deploy/action.sh` | New action script | L2 (`set -euo pipefail`), L5-19 (config), L26-32 (logging), L37-64 (API helper), L307 (mask), L340-342 (outputs) | +| `/home/user/github-actions/.github/actions/arcane-deploy/action.yml` | New action definition | L1-7 (metadata+branding), L9-84 (inputs), L86-97 (outputs), L99-121 (runs+env mapping) | +| `/home/user/github-actions/.github/actions/arcane-deploy/README.md` | New action docs | Only action with a dedicated README | +| `/home/user/github-actions/.github/actions/claude-auth/action.sh` | Existing comparable script | L2 (`set -e`), L5-8 (colors), L11-21 (logging with colors), L24-41 (helpers), 4-space indent | +| `/home/user/github-actions/.github/actions/claude-auth/action.yml` | Existing comparable action.yml | L1-6 (metadata+branding), L9-69 (inputs), L71-74 (outputs), L76-96 (runs+env mapping) | +| `/home/user/github-actions/.github/actions/claude-debug/action.yml` | Existing action with inline script | L79-222 (inline bash with `set_output()`, `::group::`) | +| `/home/user/github-actions/.github/actions/interpolate-prompt/action.yml` | Existing action | L29 (`::error::` usage) | +| `/home/user/github-actions/.github/actions/github-app-auth/action.yml` | Existing action (no shell script) | L1-2 (metadata without `author`), L7-8 (branding) | +| `/home/user/github-actions/README.md` | Root docs | L92-107 (arcane-deploy section with link to action README) | From 9f3ae1d7aa33772286b97c26f5c5c0388396c034 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 23 Feb 2026 19:20:56 +0000 Subject: [PATCH 08/14] docs: add complete PR review with overall report All 8 category reviews complete plus overall summary. https://claude.ai/code/session_01PrmzWtCEcxJTMjxXje3ndd --- .../1/1771874092/OVERALL-REPORT.md | 66 ++++++++ .../1/1771874092/quality-assurance/REPORT.md | 157 ++++++++++++++++++ 2 files changed, 223 insertions(+) create mode 100644 .claude/pr-reviews/nsheaps/github-actions/1/1771874092/OVERALL-REPORT.md create mode 100644 .claude/pr-reviews/nsheaps/github-actions/1/1771874092/quality-assurance/REPORT.md diff --git a/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/OVERALL-REPORT.md b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/OVERALL-REPORT.md new file mode 100644 index 0000000..16a1977 --- /dev/null +++ b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/OVERALL-REPORT.md @@ -0,0 +1,66 @@ +# Overall PR Review: arcane-deploy GitHub Action + +## Score Summary + +| Category | Score | Status | +|---|---|---| +| Simplicity | 42/100 | :rotating_light: | +| Flexibility | 62/100 | :rotating_light: | +| Usability | 72/100 | :warning: | +| Documentation | 82/100 | :warning: | +| Security | 52/100 | :rotating_light: | +| Patterns | 62/100 | :rotating_light: | +| Best Practices | 86/100 | :white_check_mark: | +| Quality Assurance | 52/100 | :rotating_light: | +| **Overall** | **64/100** | :rotating_light: | + +## Executive Summary + +This PR introduces a well-intentioned `arcane-deploy` GitHub Action that automates Docker Compose stack deployment to Arcane via its GitOps sync API. The action follows the repo's structural conventions (composite action, `action.yml` + `action.sh`, `INPUT_*` env mapping) and demonstrates strong engineering in several areas: idempotent upsert logic, safe JSON construction via `jq --arg`, proper use of `::group::` logging, and comprehensive README documentation. + +However, the action has significant issues across multiple dimensions that warrant a round of revisions before merge: + +### Critical Issues (Must Fix) + +1. **Security: `git-token` is never masked** (`action.sh:14`) -- The git token secret is never passed through `::add-mask::`, creating a real risk of secret leakage in workflow logs. The API key is masked (line 307) but too late in the script. + +2. **Security: `GITHUB_ENV` injection via `env-vars`** (`action.sh:260-278`) -- User-supplied `KEY=VALUE` lines are written directly to `$GITHUB_ENV` with no sanitization. Multiline values or delimiter manipulation can overwrite arbitrary environment variables for subsequent workflow steps. + +3. **Security: No URL scheme validation** (`action.sh:5`) -- `arcane-url` feeds directly into `curl` with no HTTPS enforcement, enabling SSRF risk. + +4. **Correctness: `sync_name_from_path` produces collisions** (`action.sh:111-127`) -- `basename(dirname(path))` naming causes two compose files in sibling directories with the same leaf name (e.g., `web/frontend/compose.yml` and `api/frontend/compose.yml`) to produce identical sync names, with the second silently overwriting the first. + +5. **Correctness: `curl || true` swallows connection errors** (`action.sh:53`) -- Network failures produce empty `http_code`, which passes the `>= 400` check as "success", sending empty data to `jq` and causing confusing downstream parse errors. + +### Important Issues (Should Fix) + +6. **Flexibility: `maxdepth 2` is hardcoded and undocumented** (`action.sh:91`) -- Deeper directory structures are silently missed with no way to override. + +7. **Flexibility: SSH auth is declared but non-functional** (`action.yml:49-52`) -- `auth-type: ssh` is accepted but no SSH key handling exists. + +8. **Usability: `env-vars` semantics are misleading** (`action.yml:81-84`) -- Description implies compose-level injection, but it only writes to `GITHUB_ENV` for subsequent workflow steps. + +9. **Patterns: Multiple deviations from repo conventions** -- `set -euo pipefail` vs `set -e`, no colored output, no helper functions (`set_output`, `mask_value`), 2-space vs 4-space indent, second commit lacks conventional commit prefix. + +10. **QA: No curl timeout** -- Missing `--connect-timeout`/`--max-time` means the action hangs indefinitely if Arcane is unreachable. + +### What's Done Well + +- Idempotent upsert pattern matched by `composePath + repositoryId` +- Safe JSON construction with `jq --arg` throughout (no JSON injection) +- Comprehensive action README with three usage examples, inputs table, and "How It Works" section +- Clean four-phase `::group::` structure for readable logs +- Smart defaults derived from GitHub context (`GITHUB_REF_NAME`, `GITHUB_REPOSITORY`) +- First action in the repo with a dedicated README -- a good pattern to adopt + +## Detailed Category Reports + +Individual detailed reports are available in: +- [Simplicity](./simplicity/REPORT.md) +- [Flexibility](./flexibility/REPORT.md) +- [Usability](./usability/REPORT.md) +- [Documentation](./documentation/REPORT.md) +- [Security](./security/REPORT.md) +- [Patterns](./patterns/REPORT.md) +- [Best Practices](./best-practices/REPORT.md) +- [Quality Assurance](./quality-assurance/REPORT.md) diff --git a/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/quality-assurance/REPORT.md b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/quality-assurance/REPORT.md new file mode 100644 index 0000000..b29f146 --- /dev/null +++ b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/quality-assurance/REPORT.md @@ -0,0 +1,157 @@ +# Quality Assurance & Engineering Review + +**Score: 52/100** + +This action demonstrates a solid understanding of the Arcane API workflow and follows reasonable bash scripting conventions (`set -euo pipefail`, structured functions, GitHub Actions logging groups). However, it has significant correctness bugs that will cause failures in common scenarios, several robustness gaps around API error handling and edge cases, notable code duplication, and no test coverage or testable structure. The action would likely work in the happy path for a simple single-directory layout, but would fail or behave unexpectedly in many realistic edge cases. + +## Detailed Findings + +### Correctness Analysis + +**Critical: `sync_name_from_path` produces collisions for deeply nested paths (lines 111-127)** + +The naming logic uses only `basename` of the parent directory. Given these files: +- `stacks/web/frontend/compose.yml` -> name: `prefix-frontend` +- `stacks/api/frontend/compose.yml` -> name: `prefix-frontend` + +Both produce the same sync name, meaning the second will silently overwrite the first in the upsert logic. This is a data-loss scenario. The function should incorporate the full relative path into the name (e.g., slugifying the directory path) rather than only the immediate parent. + +**Critical: `sync_name_from_path` loses uniqueness when prefix matches directory name (lines 121-123)** + +If `SYNC_NAME_PREFIX` is `myapp` and the compose file is at `myapp/compose.yml`, `dirname` returns `myapp`, `basename` returns `myapp`, and the condition `name != SYNC_NAME_PREFIX` is false, so the name is just `myapp`. But if there is also a root `compose.yml`, that also gets the name `myapp` (from the `dir == "."` branch). Two distinct compose files produce the same sync name. + +**Bug: `jq` filter `first // empty` behavior with `.id` (lines 138-140, 186-189)** + +The jq expression `[.[] | select(...)] | first // empty | .id` has a subtle issue. If `first` returns an object, `.id` works fine. But if the array is empty, `first` returns `null`, then `// empty` produces nothing, and `.id` on nothing produces nothing -- which works correctly here. However, if the API ever returns an entry where `.id` is `null` or `0`, the `// empty` alternative operator would incorrectly activate because `null // empty` triggers the alternative. This is a minor concern but worth noting. + +**Bug: `repositoryId` field type mismatch potential (line 189)** + +The `existing_id` and `REPOSITORY_ID` values are extracted with `jq -r`, which returns string representations. If the Arcane API uses integer IDs, the jq `select` comparing `.repositoryId == $repoId` compares a number to a string and will never match, meaning syncs will always be created, never updated. This would cause duplicate syncs on every run. The fix would be to use `--argjson` for numeric IDs or convert within the jq filter using `tostring`. + +**Issue: `mapfile` on empty `discover_compose_files` output (line 315)** + +If `discover_compose_files` returns an empty string (which it does not, due to the early return 1), this would create an array with one empty element. The `[[ -z "${compose_path}" ]] && continue` guard on line 331 handles this, but only incidentally. More importantly, because `set -e` is active, the `return 1` on line 102 will cause the subshell on line 314 to fail and the script to exit -- but the error message alone may not be clear since the `::group::` was already opened on line 313 but never closed. + +**Issue: Explicit compose files are not validated (lines 71-78)** + +When using `compose-files` input, the paths are taken verbatim without checking they actually exist on disk. While the Arcane API might not need them to exist locally (it syncs via git), there is no validation or warning if a path is clearly wrong. This could lead to silent misconfiguration. + +**Issue: `GITHUB_WORKSPACE` fallback to `.` (lines 82, 89)** + +The fallback `${GITHUB_WORKSPACE:-.}` means if `GITHUB_WORKSPACE` is unset (e.g., local testing), the script uses the current directory. However, the relative path stripping on line 89 (`${file#"${GITHUB_WORKSPACE:-.}/"}`) depends on the prefix matching exactly. If `GITHUB_WORKSPACE` is set but `COMPOSE_DIR` results in a resolved symlink or different path representation, the prefix stripping will fail and full absolute paths will leak into the sync configuration. + +**Issue: `::add-mask::` after logging (lines 282-286 vs 307)** + +The API key is masked on line 307, but the script has already logged `ARCANE_URL`, `ENV_ID`, etc. on lines 282-286. While the API key itself is not logged before masking, the mask command should ideally come as early as possible -- before any output -- to ensure no accidental leakage in future code changes. + +### Robustness + +**No curl timeout (line 47)** + +The `curl` command has no `--connect-timeout` or `--max-time` flags. If the Arcane instance is unreachable or hangs, the action will hang indefinitely (until the GitHub Actions job timeout, which defaults to 6 hours). Adding `--connect-timeout 10 --max-time 60` would make failures explicit and fast. + +**No retry logic for transient failures** + +API calls can fail due to transient network issues, rate limiting, or server errors (5xx). There is no retry mechanism. A single network blip will fail the entire deployment. At minimum, the `arcane_api` function should retry on 5xx responses or connection failures. + +**`|| true` on curl swallows connection errors (line 53)** + +The `|| true` after curl means that if curl itself fails (DNS resolution failure, connection refused, etc.), `http_code` will be empty or `000`. The subsequent `[[ "${http_code}" -ge 400 ]]` comparison will treat empty/000 as success (since `000 -ge 400` is false), and the function will `cat` an empty temp file and return success with empty output. Downstream `jq` calls on this empty output will then fail with parse errors, producing confusing error messages that do not point to the actual network failure. + +A fix would be: +```bash +if [[ -z "${http_code}" || "${http_code}" == "000" ]]; then + log_error "API ${method} ${path} failed (connection error)" + rm -f "${tmp_body}" + return 1 +fi +``` + +**No validation of jq output after repository creation (line 170)** + +After creating a repository, `REPOSITORY_ID` is set from `jq -r '.id'`. If the API returns unexpected JSON (e.g., an error wrapped in 2xx), `.id` will be `null`, and `REPOSITORY_ID` will be the literal string `"null"`. All subsequent API calls will use `/environments/.../gitops-syncs/null/...`, which will likely return 404s with confusing errors. + +**`GITHUB_ENV` and `GITHUB_OUTPUT` assumed to exist (lines 275, 340-342)** + +If these environment files do not exist (e.g., when running outside GitHub Actions for testing), the script will fail with a confusing "No such file or directory" error. A guard like `[[ -n "${GITHUB_OUTPUT:-}" ]]` would improve local testability. + +**Temp file cleanup on unexpected exit (line 44)** + +The `mktemp` files in `arcane_api` are cleaned up in the normal flow, but if the script is killed (SIGTERM/SIGINT) between `mktemp` and `rm -f`, temp files will leak. A `trap` to clean up would be more robust, though this is a minor concern for ephemeral CI runners. + +**No handling of API response pagination** + +If the Arcane API paginates results (e.g., for repositories or syncs), the script only processes the first page. With many repositories or syncs, existing items might not be found, leading to duplicates. + +### Code Quality + +**Duplicated trigger-sync blocks (lines 217-220 and 252-255)** + +The trigger sync logic appears identically in both the update and create branches of `upsert_sync`. This should be extracted into a helper function or moved after the if/else block using a variable for the sync ID: + +```bash +local sync_id="${existing_id:-${new_id}}" +if [[ "${TRIGGER_SYNC}" == "true" ]]; then + log_info " Triggering sync..." + arcane_api POST "/environments/${ENV_ID}/gitops-syncs/${sync_id}/sync" > /dev/null || true +fi +``` + +**Global mutable state (lines 21-23)** + +`SYNCS_CREATED`, `SYNCS_UPDATED`, and `REPOSITORY_ID` are global variables mutated by functions. This makes the code harder to reason about and test. `REPOSITORY_ID` in particular is set as a side effect of `ensure_repository()` and then read by `upsert_sync()` via closure over the global. Returning values from functions via stdout (as `discover_compose_files` does) would be cleaner. + +**`sync_name_from_path` only uses immediate parent directory (lines 111-127)** + +As noted in correctness, this is also a design concern. The naming scheme is too simplistic for real-world use. A slugified full relative path would be more robust and predictable. + +**Environment variable export side effect (lines 260-278)** + +The `export_env_vars` function writes to `GITHUB_ENV`, which affects subsequent steps in the workflow. This is a side effect that is not clearly documented as affecting the broader workflow context, not just the current action. Users may be surprised that setting `env-vars` in this action pollutes the entire workflow's environment. + +**No shellcheck annotations or CI validation** + +The script does not appear to be validated by shellcheck in CI. Some patterns (like the whitespace trimming on lines 73-74) would benefit from shellcheck's analysis to confirm correctness. + +**Hardcoded `-maxdepth 2` in find (line 91)** + +The directory scan only goes 2 levels deep. This is undocumented behavior and will silently miss compose files in deeper directory structures. The README says "scans for compose files in a directory" without mentioning the depth limit. + +### Testing + +**No tests exist** + +There are no unit tests, integration tests, or even a test workflow for this action. For a script that makes destructive API calls (creating repositories and syncs), this is a significant gap. + +**Code is not structured for testability** + +The script uses global state, direct API calls, and GitHub Actions-specific environment variables throughout. To make this testable, you would need: + +1. Ability to mock `arcane_api` (currently a function, which is good, but it cannot be replaced without sourcing) +2. Ability to set `GITHUB_ENV`/`GITHUB_OUTPUT` to temp files +3. A way to run `discover_compose_files` in isolation against a test directory + +**Recommended test approaches:** + +- **Unit tests with BATS** (Bash Automated Testing System): Source the script's functions and test them individually. The function-based structure makes this partially feasible, but the global state and `set -euo pipefail` at the top complicate sourcing. +- **Integration tests with a mock Arcane API**: Use a simple HTTP server (e.g., `python3 -m http.server` or a purpose-built mock) to validate the full flow. +- **Dry-run mode**: Add a `dry-run` input that logs what would happen without making API calls. This would aid both testing and user confidence. + +**No validation of API contract** + +There is no schema validation of the Arcane API responses. If the API changes (field renames, type changes), failures will be cryptic jq errors rather than clear "API contract violated" messages. + +## References + +- `/home/user/github-actions/.github/actions/arcane-deploy/action.sh` -- Main script (345 lines) + - Lines 47-53: `arcane_api` curl call with `|| true` swallowing connection errors + - Lines 111-127: `sync_name_from_path` with collision risk + - Lines 138-140: jq filter for repository lookup, potential type mismatch + - Lines 186-189: jq filter for sync lookup, `$repoId` string-vs-number comparison risk + - Lines 217-220, 252-255: Duplicated trigger-sync logic + - Lines 21-23: Global mutable state variables + - Line 91: Hardcoded `-maxdepth 2` depth limit + - Line 307: `::add-mask::` called after initial logging +- `/home/user/github-actions/.github/actions/arcane-deploy/action.yml` -- Action definition (122 lines) + - Lines 99-122: Composite action with env passthrough +- `/home/user/github-actions/.github/actions/arcane-deploy/README.md` -- Documentation (98 lines) From 0b0b87e52c675e80b675feac35408fd56e4372d3 Mon Sep 17 00:00:00 2001 From: nsheaps <1282393+nsheaps@users.noreply.github.com> Date: Mon, 23 Feb 2026 19:21:58 +0000 Subject: [PATCH 09/14] chore: `mise format` Triggered by: 2afd682d0f01cf890a34f781704f1ab174c26b3c Workflow run: https://github.com/nsheaps/github-actions/actions/runs/22321236572 --- .../1/1771874092/OVERALL-REPORT.md | 23 ++++++++++--------- .../1/1771874092/quality-assurance/REPORT.md | 2 ++ 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/OVERALL-REPORT.md b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/OVERALL-REPORT.md index 16a1977..33269e5 100644 --- a/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/OVERALL-REPORT.md +++ b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/OVERALL-REPORT.md @@ -2,17 +2,17 @@ ## Score Summary -| Category | Score | Status | -|---|---|---| -| Simplicity | 42/100 | :rotating_light: | -| Flexibility | 62/100 | :rotating_light: | -| Usability | 72/100 | :warning: | -| Documentation | 82/100 | :warning: | -| Security | 52/100 | :rotating_light: | -| Patterns | 62/100 | :rotating_light: | -| Best Practices | 86/100 | :white_check_mark: | -| Quality Assurance | 52/100 | :rotating_light: | -| **Overall** | **64/100** | :rotating_light: | +| Category | Score | Status | +| ----------------- | ---------- | ------------------ | +| Simplicity | 42/100 | :rotating_light: | +| Flexibility | 62/100 | :rotating_light: | +| Usability | 72/100 | :warning: | +| Documentation | 82/100 | :warning: | +| Security | 52/100 | :rotating_light: | +| Patterns | 62/100 | :rotating_light: | +| Best Practices | 86/100 | :white_check_mark: | +| Quality Assurance | 52/100 | :rotating_light: | +| **Overall** | **64/100** | :rotating_light: | ## Executive Summary @@ -56,6 +56,7 @@ However, the action has significant issues across multiple dimensions that warra ## Detailed Category Reports Individual detailed reports are available in: + - [Simplicity](./simplicity/REPORT.md) - [Flexibility](./flexibility/REPORT.md) - [Usability](./usability/REPORT.md) diff --git a/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/quality-assurance/REPORT.md b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/quality-assurance/REPORT.md index b29f146..9d93afc 100644 --- a/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/quality-assurance/REPORT.md +++ b/.claude/pr-reviews/nsheaps/github-actions/1/1771874092/quality-assurance/REPORT.md @@ -11,6 +11,7 @@ This action demonstrates a solid understanding of the Arcane API workflow and fo **Critical: `sync_name_from_path` produces collisions for deeply nested paths (lines 111-127)** The naming logic uses only `basename` of the parent directory. Given these files: + - `stacks/web/frontend/compose.yml` -> name: `prefix-frontend` - `stacks/api/frontend/compose.yml` -> name: `prefix-frontend` @@ -59,6 +60,7 @@ API calls can fail due to transient network issues, rate limiting, or server err The `|| true` after curl means that if curl itself fails (DNS resolution failure, connection refused, etc.), `http_code` will be empty or `000`. The subsequent `[[ "${http_code}" -ge 400 ]]` comparison will treat empty/000 as success (since `000 -ge 400` is false), and the function will `cat` an empty temp file and return success with empty output. Downstream `jq` calls on this empty output will then fail with parse errors, producing confusing error messages that do not point to the actual network failure. A fix would be: + ```bash if [[ -z "${http_code}" || "${http_code}" == "000" ]]; then log_error "API ${method} ${path} failed (connection error)" From aa1e70c6aeb1cae9ed5f14af3c5034cb4ad80329 Mon Sep 17 00:00:00 2001 From: Nathan Heaps <1282393+nsheaps@users.noreply.github.com> Date: Mon, 23 Feb 2026 20:36:50 -0500 Subject: [PATCH 10/14] =?UTF-8?q?fix:=20address=20all=20review=20findings?= =?UTF-8?q?=20=E2=80=94=20security,=20reliability,=20usability,=20docs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security (Critical): - C1: Move ::add-mask:: for API key to top of script, before any logging - C2: Add ::add-mask:: for git token immediately after assignment Security/Reliability (High): - H1: Mask env-var values, validate key format, reject newline injection - H2: Replace curl || true with proper transport error handling + timeouts - H3: Validate sync-interval is a positive integer before jq --argjson - H7: Add jq_extract_id() helper to guard against null/malformed API IDs Usability (High): - H4: Reject unsupported auth-type values (remove ssh until implemented) - H5: Validate git-token is required when auth-type is http (the default) - H6: Correct env-vars description — these are runner env vars, not container env Simplicity: - Deduplicate trigger-sync logic (was in both if/else branches) - Remove unnecessary auto_sync_val intermediate variable - Extract trim() and require_input() helper functions - Add comment explaining sync name dedup logic Co-Authored-By: Claude Opus 4.6 --- .github/actions/arcane-deploy/README.md | 10 +- .github/actions/arcane-deploy/action.sh | 149 +++++++++++++++++------ .github/actions/arcane-deploy/action.yml | 8 +- 3 files changed, 120 insertions(+), 47 deletions(-) diff --git a/.github/actions/arcane-deploy/README.md b/.github/actions/arcane-deploy/README.md index 14b02ab..e85e33d 100644 --- a/.github/actions/arcane-deploy/README.md +++ b/.github/actions/arcane-deploy/README.md @@ -8,7 +8,7 @@ Deploy Docker Compose stacks to [Arcane](https://github.com/getarcaneapp/arcane) - Explicit compose file list for full control - Creates the git repository in Arcane if it doesn't exist - Creates or updates gitops syncs (never deletes) -- Shared environment variables across stacks +- Workflow environment variables for subsequent steps - Triggers immediate sync after changes ## Usage @@ -42,7 +42,7 @@ Deploy Docker Compose stacks to [Arcane](https://github.com/getarcaneapp/arcane) git-token: ${{ secrets.REPO_TOKEN }} ``` -**With shared environment variables:** +**With workflow environment variables** (available to subsequent steps, not inside containers): ```yaml - name: Deploy stacks to Arcane @@ -71,13 +71,13 @@ Deploy Docker Compose stacks to [Arcane](https://github.com/getarcaneapp/arcane) | `repository-url` | No | GitHub repo HTTPS URL | Git URL for Arcane to clone | | `repository-name` | No | GitHub repo name | Display name in Arcane | | `branch` | No | Triggering branch | Branch to sync from | -| `auth-type` | No | `http` | Git auth type: `none`, `http`, or `ssh` | -| `git-token` | No | | Token for HTTP git authentication | +| `auth-type` | No | `http` | Git auth type: `none` or `http` | +| `git-token` | No | | Token for HTTP git auth. Required when auth-type=http. | | `auto-sync` | No | `true` | Enable Arcane auto-sync polling | | `sync-interval` | No | `5` | Minutes between auto-sync polls | | `trigger-sync` | No | `true` | Trigger immediate sync after create/update | | `sync-name-prefix` | No | GitHub repo name | Prefix for sync names in Arcane | -| `env-vars` | No | | Shared env vars (`KEY=VALUE` per line) for the workflow | +| `env-vars` | No | | Runner env vars (`KEY=VALUE` per line) for subsequent steps. Values are masked. | ## Outputs diff --git a/.github/actions/arcane-deploy/action.sh b/.github/actions/arcane-deploy/action.sh index b7dda85..f197083 100755 --- a/.github/actions/arcane-deploy/action.sh +++ b/.github/actions/arcane-deploy/action.sh @@ -18,11 +18,16 @@ TRIGGER_SYNC="${INPUT_TRIGGER_SYNC:-true}" ENV_VARS="${INPUT_ENV_VARS:-}" SYNC_NAME_PREFIX="${INPUT_SYNC_NAME_PREFIX:-${GITHUB_REPOSITORY##*/}}" +# [C1/C2] Mask secrets immediately, before any logging or API calls +[[ -n "${API_KEY}" ]] && echo "::add-mask::${API_KEY}" +[[ -n "${GIT_TOKEN}" ]] && echo "::add-mask::${GIT_TOKEN}" + SYNCS_CREATED=0 SYNCS_UPDATED=0 REPOSITORY_ID="" -# --- Logging --- +# --- Helpers --- + log_info() { echo "[Arcane Deploy] $1" } @@ -31,6 +36,23 @@ log_error() { echo "::error::[Arcane Deploy] $1" } +# Trim leading and trailing whitespace from a string. +trim() { + local s="$1" + s="${s#"${s%%[![:space:]]*}"}" + s="${s%"${s##*[![:space:]]}"}" + echo "$s" +} + +# Validate a required input is non-empty. +require_input() { + local name="$1" value="$2" + if [[ -z "${value}" ]]; then + log_error "${name} is required" + exit 1 + fi +} + # --- API Helper --- # Make an authenticated API request to Arcane. # Usage: arcane_api METHOD /path [curl args...] @@ -44,13 +66,26 @@ arcane_api() { tmp_body=$(mktemp) local http_code + # [H2] Capture curl exit code separately to detect transport failures http_code=$(curl -s -w "%{http_code}" \ + --max-time 30 --connect-timeout 10 \ -X "${method}" \ -H "X-Api-Key: ${API_KEY}" \ -H "Content-Type: application/json" \ -o "${tmp_body}" \ "$@" \ - "${url}") || true + "${url}") || { + log_error "API ${method} ${path} failed: curl transport error (DNS, timeout, or connection refused)" + rm -f "${tmp_body}" + return 1 + } + + # [H2] Guard against empty http_code (should not happen after above, but be safe) + if [[ -z "${http_code}" ]]; then + log_error "API ${method} ${path} failed: no HTTP response code received" + rm -f "${tmp_body}" + return 1 + fi if [[ "${http_code}" -ge 400 ]]; then log_error "API ${method} ${path} failed (HTTP ${http_code})" @@ -63,6 +98,23 @@ arcane_api() { rm -f "${tmp_body}" } +# [H7] Extract a jq field and validate it is non-empty and non-"null". +# Usage: jq_extract_id JSON_STRING JQ_FILTER CONTEXT_MSG +# Returns the extracted value on stdout, or returns 1 on failure. +jq_extract_id() { + local json="$1" filter="$2" context="$3" + local value + value=$(echo "${json}" | jq -r "${filter}" 2>/dev/null) || { + log_error "${context}: failed to parse API response as JSON" + return 1 + } + if [[ -z "${value}" || "${value}" == "null" ]]; then + log_error "${context}: expected a valid ID but got '${value}'" + return 1 + fi + echo "${value}" +} + # --- Compose File Discovery --- discover_compose_files() { local files=() @@ -70,8 +122,7 @@ discover_compose_files() { # From explicit list if [[ -n "${COMPOSE_FILES_INPUT}" ]]; then while IFS= read -r file; do - file="${file#"${file%%[![:space:]]*}"}" # trim leading - file="${file%"${file##*[![:space:]]}"}" # trim trailing + file="$(trim "${file}")" [[ -z "${file}" ]] && continue files+=("${file}") done <<< "${COMPOSE_FILES_INPUT}" @@ -118,6 +169,7 @@ sync_name_from_path() { name="${SYNC_NAME_PREFIX}" else name=$(basename "${dir}") + # Avoid "prefix-prefix" when the directory name matches the prefix if [[ "${name}" != "${SYNC_NAME_PREFIX}" ]]; then name="${SYNC_NAME_PREFIX}-${name}" fi @@ -139,7 +191,7 @@ ensure_repository() { --arg url "${REPO_URL}" \ '[.[] | select(.url == $url)] | first // empty | .id') - if [[ -n "${REPOSITORY_ID}" ]]; then + if [[ -n "${REPOSITORY_ID}" && "${REPOSITORY_ID}" != "null" ]]; then log_info "Found existing repository: ${REPOSITORY_ID}" # Update credentials so the token stays current @@ -167,7 +219,8 @@ ensure_repository() { local result result=$(arcane_api POST "/customize/git-repositories" -d "${create_payload}") - REPOSITORY_ID=$(echo "${result}" | jq -r '.id') + # [H7] Validate the response contains a real ID + REPOSITORY_ID=$(jq_extract_id "${result}" '.id' "create repository") log_info "Created repository: ${REPOSITORY_ID}" fi @@ -188,10 +241,8 @@ upsert_sync() { --arg repoId "${REPOSITORY_ID}" \ '[.[] | select(.composePath == $composePath and .repositoryId == $repoId)] | first // empty | .id') - local auto_sync_val="false" - [[ "${AUTO_SYNC}" == "true" ]] && auto_sync_val="true" - - if [[ -n "${existing_id}" ]]; then + local sync_id + if [[ -n "${existing_id}" && "${existing_id}" != "null" ]]; then log_info " Updating sync ${existing_id} (${sync_name})" local update_payload @@ -199,7 +250,7 @@ upsert_sync() { --arg name "${sync_name}" \ --arg branch "${BRANCH}" \ --arg composePath "${compose_path}" \ - --argjson autoSync "${auto_sync_val}" \ + --argjson autoSync "${AUTO_SYNC}" \ --argjson syncInterval "${SYNC_INTERVAL}" \ '{ name: $name, @@ -213,11 +264,7 @@ upsert_sync() { -d "${update_payload}" > /dev/null SYNCS_UPDATED=$((SYNCS_UPDATED + 1)) - - if [[ "${TRIGGER_SYNC}" == "true" ]]; then - log_info " Triggering sync..." - arcane_api POST "/environments/${ENV_ID}/gitops-syncs/${existing_id}/sync" > /dev/null || true - fi + sync_id="${existing_id}" else log_info " Creating sync: ${sync_name}" @@ -228,7 +275,7 @@ upsert_sync() { --arg branch "${BRANCH}" \ --arg composePath "${compose_path}" \ --arg projectName "${sync_name}" \ - --argjson autoSync "${auto_sync_val}" \ + --argjson autoSync "${AUTO_SYNC}" \ --argjson syncInterval "${SYNC_INTERVAL}" \ '{ name: $name, @@ -243,34 +290,53 @@ upsert_sync() { local result result=$(arcane_api POST "/environments/${ENV_ID}/gitops-syncs" -d "${create_payload}") - local new_id - new_id=$(echo "${result}" | jq -r '.id') - log_info " Created sync: ${new_id}" + # [H7] Validate the response contains a real ID + sync_id=$(jq_extract_id "${result}" '.id' "create sync '${sync_name}'") + log_info " Created sync: ${sync_id}" SYNCS_CREATED=$((SYNCS_CREATED + 1)) + fi - if [[ "${TRIGGER_SYNC}" == "true" ]]; then - log_info " Triggering sync..." - arcane_api POST "/environments/${ENV_ID}/gitops-syncs/${new_id}/sync" > /dev/null || true - fi + # Deduplicated trigger-sync (was duplicated in both branches) + if [[ "${TRIGGER_SYNC}" == "true" ]]; then + log_info " Triggering sync..." + arcane_api POST "/environments/${ENV_ID}/gitops-syncs/${sync_id}/sync" > /dev/null || true fi } # --- Environment Variables --- +# Export variables to the GitHub Actions runner environment ($GITHUB_ENV). +# NOTE: These are CI workflow-scoped variables, NOT Arcane deployment-time variables. +# They are available to subsequent workflow steps, not inside deployed containers. export_env_vars() { if [[ -z "${ENV_VARS}" ]]; then return fi - echo "::group::Setting shared environment variables" + echo "::group::Setting workflow environment variables" while IFS= read -r line; do - line="${line#"${line%%[![:space:]]*}"}" # trim leading - line="${line%"${line##*[![:space:]]}"}" # trim trailing + line="$(trim "${line}")" [[ -z "${line}" ]] && continue [[ "${line}" == \#* ]] && continue local key="${line%%=*}" local value="${line#*=}" + + # [H1] Validate key format to prevent env injection via malformed keys + if [[ ! "${key}" =~ ^[A-Za-z_][A-Za-z0-9_]*$ ]]; then + log_error "Invalid env-var key: '${key}' (must match [A-Za-z_][A-Za-z0-9_]*)" + exit 1 + fi + + # [H1] Reject embedded newlines in values to prevent env injection + if [[ "${value}" == *$'\n'* ]]; then + log_error "Invalid env-var value for '${key}': embedded newlines are not allowed" + exit 1 + fi + + # [H1] Mask the value so it does not appear in logs of subsequent steps + echo "::add-mask::${value}" + log_info " ${key}=***" echo "${key}=${value}" >> "${GITHUB_ENV}" done <<< "${ENV_VARS}" @@ -286,26 +352,33 @@ log_info "Repository: ${REPO_URL}" log_info "Branch: ${BRANCH}" # Validate required inputs -if [[ -z "${ARCANE_URL}" ]]; then - log_error "arcane-url is required" +require_input "arcane-url" "${ARCANE_URL}" +require_input "arcane-api-key" "${API_KEY}" +require_input "environment-id" "${ENV_ID}" + +if [[ -z "${COMPOSE_DIR}" && -z "${COMPOSE_FILES_INPUT}" ]]; then + log_error "At least one of compose-dir or compose-files is required" exit 1 fi -if [[ -z "${API_KEY}" ]]; then - log_error "arcane-api-key is required" + +# [H5] Validate git-token is set when auth-type is http +if [[ "${AUTH_TYPE}" == "http" && -z "${GIT_TOKEN}" ]]; then + log_error "git-token is required when auth-type is http (the default). Set git-token or use auth-type: none." exit 1 fi -if [[ -z "${ENV_ID}" ]]; then - log_error "environment-id is required" + +# [H4] Reject unsupported auth-type values +if [[ "${AUTH_TYPE}" != "none" && "${AUTH_TYPE}" != "http" ]]; then + log_error "auth-type '${AUTH_TYPE}' is not supported. Valid values: none, http." exit 1 fi -if [[ -z "${COMPOSE_DIR}" && -z "${COMPOSE_FILES_INPUT}" ]]; then - log_error "At least one of compose-dir or compose-files is required" + +# [H3] Validate sync-interval is a positive integer +if [[ ! "${SYNC_INTERVAL}" =~ ^[1-9][0-9]*$ ]]; then + log_error "sync-interval must be a positive integer, got '${SYNC_INTERVAL}'" exit 1 fi -# Mask the API key -echo "::add-mask::${API_KEY}" - # Export shared env vars export_env_vars diff --git a/.github/actions/arcane-deploy/action.yml b/.github/actions/arcane-deploy/action.yml index a9a2869..39e4516 100644 --- a/.github/actions/arcane-deploy/action.yml +++ b/.github/actions/arcane-deploy/action.yml @@ -47,12 +47,12 @@ inputs: default: '' auth-type: - description: 'Git authentication type: none, http, or ssh' + description: 'Git authentication type: none or http' required: false default: 'http' git-token: - description: 'Token for HTTP git authentication. Required when auth-type is http.' + description: 'Token for HTTP git authentication. Required when auth-type is http (the default).' required: false default: '' @@ -77,9 +77,9 @@ inputs: required: false default: '' - # Shared environment variables + # Workflow environment variables env-vars: - description: 'Shared environment variables (KEY=VALUE per line) to export for the workflow. Use these in your compose files via variable interpolation or .env files in your repo.' + description: 'Environment variables (KEY=VALUE per line) to export to the GitHub Actions runner via $GITHUB_ENV. These are available to subsequent workflow steps, NOT inside deployed containers. Values are automatically masked in logs.' required: false default: '' From 1b796ed88c0d1524b6f47ee169264e302d750351 Mon Sep 17 00:00:00 2001 From: nsheaps <1282393+nsheaps@users.noreply.github.com> Date: Tue, 24 Feb 2026 01:37:27 +0000 Subject: [PATCH 11/14] chore: `mise format` Triggered by: d556cfd8755955b09d80d5bf118a3faee17492b1 Workflow run: https://github.com/nsheaps/github-actions/actions/runs/22332835231 --- .github/actions/arcane-deploy/README.md | 32 ++++++++++++------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/.github/actions/arcane-deploy/README.md b/.github/actions/arcane-deploy/README.md index e85e33d..dcd2c73 100644 --- a/.github/actions/arcane-deploy/README.md +++ b/.github/actions/arcane-deploy/README.md @@ -61,22 +61,22 @@ Deploy Docker Compose stacks to [Arcane](https://github.com/getarcaneapp/arcane) ## Inputs -| Input | Required | Default | Description | -| ------------------ | -------- | --------------------- | ------------------------------------------------------- | -| `arcane-url` | Yes | | Base URL of the Arcane instance | -| `arcane-api-key` | Yes | | API key (from Arcane Settings > API Keys) | -| `environment-id` | Yes | | Arcane environment ID | -| `compose-dir` | No | | Directory to scan for compose files | -| `compose-files` | No | | Newline-separated list of compose file paths | -| `repository-url` | No | GitHub repo HTTPS URL | Git URL for Arcane to clone | -| `repository-name` | No | GitHub repo name | Display name in Arcane | -| `branch` | No | Triggering branch | Branch to sync from | -| `auth-type` | No | `http` | Git auth type: `none` or `http` | -| `git-token` | No | | Token for HTTP git auth. Required when auth-type=http. | -| `auto-sync` | No | `true` | Enable Arcane auto-sync polling | -| `sync-interval` | No | `5` | Minutes between auto-sync polls | -| `trigger-sync` | No | `true` | Trigger immediate sync after create/update | -| `sync-name-prefix` | No | GitHub repo name | Prefix for sync names in Arcane | +| Input | Required | Default | Description | +| ------------------ | -------- | --------------------- | ------------------------------------------------------------------------------- | +| `arcane-url` | Yes | | Base URL of the Arcane instance | +| `arcane-api-key` | Yes | | API key (from Arcane Settings > API Keys) | +| `environment-id` | Yes | | Arcane environment ID | +| `compose-dir` | No | | Directory to scan for compose files | +| `compose-files` | No | | Newline-separated list of compose file paths | +| `repository-url` | No | GitHub repo HTTPS URL | Git URL for Arcane to clone | +| `repository-name` | No | GitHub repo name | Display name in Arcane | +| `branch` | No | Triggering branch | Branch to sync from | +| `auth-type` | No | `http` | Git auth type: `none` or `http` | +| `git-token` | No | | Token for HTTP git auth. Required when auth-type=http. | +| `auto-sync` | No | `true` | Enable Arcane auto-sync polling | +| `sync-interval` | No | `5` | Minutes between auto-sync polls | +| `trigger-sync` | No | `true` | Trigger immediate sync after create/update | +| `sync-name-prefix` | No | GitHub repo name | Prefix for sync names in Arcane | | `env-vars` | No | | Runner env vars (`KEY=VALUE` per line) for subsequent steps. Values are masked. | ## Outputs From 430952cfb02136a33bbcb55f531f15e633fe1f29 Mon Sep 17 00:00:00 2001 From: Nathan Heaps <1282393+nsheaps@users.noreply.github.com> Date: Mon, 23 Feb 2026 20:47:50 -0500 Subject: [PATCH 12/14] fix: validate auto-sync boolean, enforce HTTPS, document scan depth - Validate auto-sync is 'true' or 'false' before passing to jq --argjson - Reject arcane-url values that don't use HTTPS - Document maxdepth 2 scan limit in compose-dir descriptions (action.yml, README) Co-Authored-By: Claude Opus 4.6 --- .github/actions/arcane-deploy/README.md | 6 +++--- .github/actions/arcane-deploy/action.sh | 12 ++++++++++++ .github/actions/arcane-deploy/action.yml | 2 +- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/.github/actions/arcane-deploy/README.md b/.github/actions/arcane-deploy/README.md index dcd2c73..e3f2602 100644 --- a/.github/actions/arcane-deploy/README.md +++ b/.github/actions/arcane-deploy/README.md @@ -63,10 +63,10 @@ Deploy Docker Compose stacks to [Arcane](https://github.com/getarcaneapp/arcane) | Input | Required | Default | Description | | ------------------ | -------- | --------------------- | ------------------------------------------------------------------------------- | -| `arcane-url` | Yes | | Base URL of the Arcane instance | +| `arcane-url` | Yes | | Base URL of the Arcane instance (must use HTTPS) | | `arcane-api-key` | Yes | | API key (from Arcane Settings > API Keys) | | `environment-id` | Yes | | Arcane environment ID | -| `compose-dir` | No | | Directory to scan for compose files | +| `compose-dir` | No | | Directory to scan for compose files (up to 2 levels deep) | | `compose-files` | No | | Newline-separated list of compose file paths | | `repository-url` | No | GitHub repo HTTPS URL | Git URL for Arcane to clone | | `repository-name` | No | GitHub repo name | Display name in Arcane | @@ -77,7 +77,7 @@ Deploy Docker Compose stacks to [Arcane](https://github.com/getarcaneapp/arcane) | `sync-interval` | No | `5` | Minutes between auto-sync polls | | `trigger-sync` | No | `true` | Trigger immediate sync after create/update | | `sync-name-prefix` | No | GitHub repo name | Prefix for sync names in Arcane | -| `env-vars` | No | | Runner env vars (`KEY=VALUE` per line) for subsequent steps. Values are masked. | +| `env-vars` | No | | Runner env vars (`KEY=VALUE` per line) for subsequent steps. Values are masked. | ## Outputs diff --git a/.github/actions/arcane-deploy/action.sh b/.github/actions/arcane-deploy/action.sh index f197083..df761d4 100755 --- a/.github/actions/arcane-deploy/action.sh +++ b/.github/actions/arcane-deploy/action.sh @@ -373,12 +373,24 @@ if [[ "${AUTH_TYPE}" != "none" && "${AUTH_TYPE}" != "http" ]]; then exit 1 fi +# Validate auto-sync is a boolean (required for jq --argjson) +if [[ "${AUTO_SYNC}" != "true" && "${AUTO_SYNC}" != "false" ]]; then + log_error "auto-sync must be 'true' or 'false', got '${AUTO_SYNC}'" + exit 1 +fi + # [H3] Validate sync-interval is a positive integer if [[ ! "${SYNC_INTERVAL}" =~ ^[1-9][0-9]*$ ]]; then log_error "sync-interval must be a positive integer, got '${SYNC_INTERVAL}'" exit 1 fi +# Validate arcane-url uses HTTPS +if [[ "${ARCANE_URL}" != https://* ]]; then + log_error "arcane-url must use HTTPS (got '${ARCANE_URL}')" + exit 1 +fi + # Export shared env vars export_env_vars diff --git a/.github/actions/arcane-deploy/action.yml b/.github/actions/arcane-deploy/action.yml index 39e4516..ddbd567 100644 --- a/.github/actions/arcane-deploy/action.yml +++ b/.github/actions/arcane-deploy/action.yml @@ -21,7 +21,7 @@ inputs: # Compose file discovery compose-dir: - description: 'Directory to scan for compose files (relative to repo root). Files matching compose.y[a]ml or docker-compose.y[a]ml are auto-discovered.' + description: 'Directory to scan for compose files (relative to repo root, up to 2 levels deep). Files matching compose.y[a]ml or docker-compose.y[a]ml are auto-discovered.' required: false default: '' From 841855fac0193f24e407947ec4b17613c8ae90bf Mon Sep 17 00:00:00 2001 From: nsheaps <1282393+nsheaps@users.noreply.github.com> Date: Tue, 24 Feb 2026 01:49:01 +0000 Subject: [PATCH 13/14] chore: `mise format` Triggered by: 11bdea19e41ac91fe89d6d601c9f9a4b03b4cca5 Workflow run: https://github.com/nsheaps/github-actions/actions/runs/22333108862 --- .github/actions/arcane-deploy/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/actions/arcane-deploy/README.md b/.github/actions/arcane-deploy/README.md index e3f2602..1659f13 100644 --- a/.github/actions/arcane-deploy/README.md +++ b/.github/actions/arcane-deploy/README.md @@ -77,7 +77,7 @@ Deploy Docker Compose stacks to [Arcane](https://github.com/getarcaneapp/arcane) | `sync-interval` | No | `5` | Minutes between auto-sync polls | | `trigger-sync` | No | `true` | Trigger immediate sync after create/update | | `sync-name-prefix` | No | GitHub repo name | Prefix for sync names in Arcane | -| `env-vars` | No | | Runner env vars (`KEY=VALUE` per line) for subsequent steps. Values are masked. | +| `env-vars` | No | | Runner env vars (`KEY=VALUE` per line) for subsequent steps. Values are masked. | ## Outputs From 60112fb789d7222eea1ed5816b769e24fec14cc5 Mon Sep 17 00:00:00 2001 From: Nathan Heaps <1282393+nsheaps@users.noreply.github.com> Date: Mon, 23 Feb 2026 20:58:56 -0500 Subject: [PATCH 14/14] fix: omit token for auth-type=none, deduplicate compose files, log trigger failures - M7: Omit token field from create-repository payload when auth-type=none - M5: Deduplicate compose files when compose-dir scan overlaps explicit list - N2: Log a warning when trigger-sync API call fails instead of silently continuing Co-Authored-By: Claude Opus 4.6 --- .github/actions/arcane-deploy/action.sh | 36 +++++++++++++++++++------ 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/.github/actions/arcane-deploy/action.sh b/.github/actions/arcane-deploy/action.sh index df761d4..f6770bc 100755 --- a/.github/actions/arcane-deploy/action.sh +++ b/.github/actions/arcane-deploy/action.sh @@ -152,7 +152,17 @@ discover_compose_files() { return 1 fi - printf '%s\n' "${files[@]}" + # Deduplicate in case compose-dir scan overlaps with explicit compose-files + local seen=() + for f in "${files[@]}"; do + local dup=false + for s in "${seen[@]+"${seen[@]}"}"; do + [[ "$s" == "$f" ]] && { dup=true; break; } + done + $dup || seen+=("$f") + done + + printf '%s\n' "${seen[@]}" } # --- Sync Naming --- @@ -209,12 +219,20 @@ ensure_repository() { log_info "Creating new repository: ${REPO_NAME}" local create_payload - create_payload=$(jq -n \ - --arg name "${REPO_NAME}" \ - --arg url "${REPO_URL}" \ - --arg authType "${AUTH_TYPE}" \ - --arg token "${GIT_TOKEN}" \ - '{name: $name, url: $url, authType: $authType, token: $token}') + if [[ "${AUTH_TYPE}" == "http" ]]; then + create_payload=$(jq -n \ + --arg name "${REPO_NAME}" \ + --arg url "${REPO_URL}" \ + --arg authType "${AUTH_TYPE}" \ + --arg token "${GIT_TOKEN}" \ + '{name: $name, url: $url, authType: $authType, token: $token}') + else + create_payload=$(jq -n \ + --arg name "${REPO_NAME}" \ + --arg url "${REPO_URL}" \ + --arg authType "${AUTH_TYPE}" \ + '{name: $name, url: $url, authType: $authType}') + fi local result result=$(arcane_api POST "/customize/git-repositories" -d "${create_payload}") @@ -300,7 +318,9 @@ upsert_sync() { # Deduplicated trigger-sync (was duplicated in both branches) if [[ "${TRIGGER_SYNC}" == "true" ]]; then log_info " Triggering sync..." - arcane_api POST "/environments/${ENV_ID}/gitops-syncs/${sync_id}/sync" > /dev/null || true + if ! arcane_api POST "/environments/${ENV_ID}/gitops-syncs/${sync_id}/sync" > /dev/null; then + log_info " Warning: trigger-sync failed for ${sync_name} (sync was still created/updated)" + fi fi }