Skip to content

fix: don't let empty ZAD_API_URL override HTTPS default#2

Closed
anneschuth wants to merge 8 commits into
mainfrom
feat/api-monitoring-backwards-compat
Closed

fix: don't let empty ZAD_API_URL override HTTPS default#2
anneschuth wants to merge 8 commits into
mainfrom
feat/api-monitoring-backwards-compat

Conversation

@anneschuth

Copy link
Copy Markdown
Member

Summary

  • When ZAD_API_URL secret is not configured, the workflow set the env var to an empty string, overriding the HTTPS default in fetch_openapi.py
  • This caused the API sync workflow to fail with "refusing to send API key over non-HTTPS connection"
  • Fix: unset the env var when it's empty so the script falls back to its built-in default

Test plan

  • Manually trigger API Sync workflow and verify it passes the fetch step

…ment

Three-layer approach to keep zad-cli in sync with the Operations Manager API:

1. oasdiff-based detection: scheduled workflow fetches the upstream OpenAPI
   spec daily, diffs it against a stored baseline, and identifies gaps.

2. Coverage script: compares upstream endpoints against ZadClient methods
   to find what the CLI doesn't cover yet.

3. Claude auto-implementation: when gaps are found, Claude implements new
   client methods, CLI commands, and tests, creating a PR for review.

Also adds:
- Backwards compatibility policy in CLAUDE.md
- CI test (test_backwards_compat.py) that fails if commands or methods
  are removed
- @claude workflow for issue/PR follow-up
- Claude code review workflow for PR review with compat focus
…nc prompt

The autonomous API sync agent needs binding design rules to produce
consistent CLI additions. Without these, it would have to reverse-engineer
patterns from the codebase and would inevitably miss conventions.

CLAUDE.md now has a full "CLI Design Principles" section covering:
- Verb vocabulary with exact semantics (list/create/add/delete/describe/etc)
- Argument rules (when positional vs option, -d only as filter)
- Destructive command pattern (--dry-run before --yes, confirm_action, etc)
- Output conventions (formatter.render, stdout vs stderr)
- Help text format (Rich markup, examples)
- Command implementation template referencing service.py
- Client method conventions (async vs sync, naming)

The api-sync.yml prompt now explicitly:
- Forces reading CLAUDE.md before implementation
- Lists which files to study as reference
- Specifies which endpoints to skip (auth, health, web UI)
- Details every mandatory layer (client, model, command, test)
- Inlines the critical rules that must not be violated
api-sync.yml:
- Add uv/Python/deps install to implement job (was missing, Claude
  couldn't run pytest)
- Fix heredoc quoting: use unquoted EOF so $(date) and env vars expand
- Add timestamp suffix to branch name to avoid same-day collisions
- Pass diff/coverage via env vars instead of inline in heredoc

check_coverage.py:
- Fix /projects/ skip prefix that would incorrectly skip /api/projects/
  after path normalization. Now only skips web UI routes (paths without
  /api/ prefix)
- Fix f-string param regex: use simple {param} replacement instead of
  broken split/rstrip logic

fetch_openapi.py:
- Add content-type check to catch non-JSON responses (e.g. login pages
  when API key is wrong)

claude-code-review.yml:
- Add Step 0 cleanup of previous claude[bot] reviews (prevents pileup)
- Reference CLI Design Principles section (not just backwards compat)
- Flag design principle violations as Critical severity

upstream-openapi.json:
- Remove non-standard _comment field, use info.description instead
Replace placeholder spec with real upstream OpenAPI spec (56 paths).

Coverage script improvements:
- Skip deprecated v1 endpoints that have v2 replacements (CLI uses v2)
- Keep deprecated v1 endpoints WITHOUT v2 replacement as "current"
  (e.g. delete_project, subdomains, validate_clone, registries)
- Use OpenAPI tags to identify deprecated endpoints instead of
  fragile path-based heuristics
- Fix path normalization: handle :action vs /:action difference
  between client code and OpenAPI spec (cancel_task was false negative)
- Show endpoint summaries and tags in uncovered output

Current coverage: 33/47 current endpoints (70%), 14 uncovered.
The 14 uncovered are: federation, admin, registries, image push,
backup-all, backup-pvc, restore-deployment, pvc-snapshots, create-task.
Four fixes:
- Add git push step after Claude implements endpoints, so commits
  actually appear on the remote branch before gh pr create runs
- Fix heredoc indentation in PR/issue body templates that caused
  all markdown to render as code blocks (10-space prefix)
- Fail the job when oasdiff crashes instead of treating the error
  as "changes detected" and triggering Claude on garbage input
- Preserve sticky comment during review cleanup by filtering out
  comments containing the claude-code-action marker
… positives

- api-sync.yml: fix oasdiff breaking exit code logic (exit 1 = breaking,
  not exit 0), use random heredoc delimiters to prevent injection via
  upstream API spec content, use --body-file for PR/issue creation,
  remove duplicate git push step
- test_backwards_compat.py: replace substring matching with structural
  parsing of Typer Rich panel output, add method signature stability test
  that checks required positional argument counts
- check_coverage.py: add sanity check that warns when regex matches
  fewer endpoints than expected (detects silent regex breakage)
- fetch_openapi.py: warn when sending API key over non-HTTPS, fix
  case-sensitive content-type check
- claude-code-review.yml: use full clone (fetch-depth: 0) so Claude
  can use git log/blame during reviews
- Pin oasdiff to v1.13.1 with SHA256 checksum verification instead of
  curl-pipe-sh from main branch (supply chain risk)
- Sanitize oasdiff/coverage output before injecting into Claude prompt
  to defend against prompt injection via malicious upstream OpenAPI specs
- Enforce HTTPS for API key transmission in fetch_openapi.py, with
  explicit --allow-insecure opt-in instead of a warning-only check
When ZAD_API_URL secret is not set, the env var was set to an empty
string, which overrode the built-in HTTPS default in fetch_openapi.py
and triggered the insecure connection check.
@anneschuth

Copy link
Copy Markdown
Member Author

Superseded by new PR from clean branch

@anneschuth anneschuth closed this Apr 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant