Skip to content

ci: sync-fork-dev uses the narrowed sync-fork key and scoped order lookup - #31

Merged
eyevinn-developer[bot] merged 1 commit into
mainfrom
ci-sync-fork-dev-narrow-key
Nov 1, 2026
Merged

eyevinn-developer[bot] merged 1 commit into
mainfrom
ci-sync-fork-dev-narrow-key

Conversation

@eyevinn-developer

Copy link
Copy Markdown
Contributor

Part of Eyevinn/osaas-maker-manager#251

Summary

.github/workflows/sync-fork-dev.yml now runs on the narrowed sync-fork key (OSC_DEV_SYNC_FORK_API_KEY) and no longer calls GET /maker (the narrow key gets 401 there).

  • Secret read from OSC_DEV_SYNC_FORK_API_KEY into SYNC_FORK_API_KEY; no fallback to OSC_DEV_MAKER_API_KEY; clear error Secret OSC_DEV_SYNC_FORK_API_KEY is not set.
  • Both GET /maker?repositoryUrl= calls replaced with GET /sync-fork/orders?repositoryUrl=. The scoped route returns the same wrapped shape as /maker ([{"order":{"orderId","status":{"phase"}}}], src/api_sync_fork_scoped.ts schema SyncForkOrderLookupResponse), so the jq paths stay .[0].order.orderId and .[0].order.status.phase.
  • More than one order for the repository URL fails loudly (Expected at most one order for $REPO_URL, found $COUNT) in both steps. All other behaviour is kept: "order before sync", same-order-id check ("Order was not remade"), DONE / FAILED / ERROR handling, 20 s polling and timeouts.
  • curl -sSf on the order lookup and on POST /sync-fork so HTTP status (for example 403 or 429 for the narrow key) shows in the log. Workflow name: unchanged (Sync fork in dev).
  • Prod workflow (sync-fork-prod.yml) is out of scope (follow-up Eyevinn/osaas-maker-manager#256).

Follow-ups

  • Allow-list for agent dispatch in python-, golang- and php-runner: Eyevinn/agent-team-osc-dev#355
  • Dev verification and closer of #251: Eyevinn/osaas-maker-manager#255
  • Prod workflows: Eyevinn/osaas-maker-manager#256

Tests (no network, scratchpad harness, nothing committed here)

A harness puts a fake curl and a no-op sleep first on PATH, extracts each step's run: script from the workflow and runs it with fake values. Samples: the one-order response captured from dev ([{"order":{"orderId":"dca57768-f713-449e-a311-5c86c224335c","status":{"phase":"DONE"}}}]) and a two-order copy. Results (20 checks, all ok):

  • bash -n on all three extracted scripts: ok
  • trigger step, one order: exit 0, prev_order_id=dca57768-... and job_id written to $GITHUB_OUTPUT
  • trigger step, two orders: exit 1, Expected at most one order
  • empty secret: exit 1, Secret OSC_DEV_SYNC_FORK_API_KEY is not set
  • remake loop: new id with DONE exits 0; same id exits 1 Order was not remade; two orders exits 1 Expected at most one order; empty list exits 1
  • static: grep -c '\$MAKER_URL/maker' is 0, OSC_DEV_MAKER_API_KEY absent, OSC_DEV_SYNC_FORK_API_KEY present
    actionlint and shellcheck are not installed on the runner, so no lint was run. The file is byte-identical across the six runner repos.

What a human does next

  1. Dev: create a long random value (not equal to the platform key), store it as key sync-fork-api-key in Secret token in namespace osaas on the dev cluster, and restart maker-manager (the Deployment reads it at pod start).
  2. Create the GitHub secret OSC_DEV_SYNC_FORK_API_KEY with the same value for the six repos (web-runner, python-runner, golang-runner, wasm-runner, dotnet-runner, php-runner); an org-level secret restricted to those repos is enough.
  3. Apply any patch posted on osaas-maker-manager#251 for a repo whose push was refused; merge the PRs. A merge-triggered run before the secret exists fails at the missing-secret check; that is intended.
  4. After the runs are green (verified by Eyevinn/osaas-maker-manager#255), delete the repo secret OSC_DEV_MAKER_API_KEY from the six repos.
  5. Prod: the same for OSC_PROD_MAKER_API_KEY after Eyevinn/osaas-maker-manager#256.

Lessons

The issue said the scoped route has no order wrapper; the real schema and live dev response are wrapped like /maker, so only the URL, key and guard change and the jq paths stay as they were. The grep check must target $MAKER_URL/maker, because the host //maker.svc... also contains /maker.

Co-Authored-By: Claude noreply@anthropic.com

🤖 Generated with Claude Code

…okup

Part of Eyevinn/osaas-maker-manager#251

Co-Authored-By: Claude <noreply@anthropic.com>

@eyevinn-pr-reviewer eyevinn-pr-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Research Review

Verified

  • GET /sync-fork/orders exists in osaas-maker-manager/src/api_sync_fork_scoped.ts (main), querystring repositoryUrl (required), response schema SyncForkOrderLookupResponse = [{order:{orderId,status:{phase}}}]. The jq paths .[0].order.orderId and .[0].order.status.phase match.
  • POST /sync-fork body {repositoryUrl} and response {jobId}, and GET /sync-fork/:id returning .status, match the same file. The scoped plugin registers all three under one bearer-auth key set (platform keys plus the dedicated key).
  • The dedicated-key checks (registered-repo 403, 429 with 60s interval) apply only to isDedicatedCaller, which matches the PR's claim that curl -sSf surfaces 403/429 in the log.
  • Lessons check: ## Lessons present and filled in, with Claude trailer.
  • Adjacent-stub sweep on sync-fork-dev.yml: no TODO/FIXME/|| undefined; no leftover MAKER_API_KEY references. Workflow file is the only touched file; there are no orphan or Playwright concerns.

Needs Verification

  • sync-fork-dev.yml (remake loop): curl -sSf ... || echo '[]' still swallows a 401/403 on the order lookup for up to 120 iterations (the loop ends as NOT_FOUND). That was the previous behaviour, so it is not a regression. Because -S is now set, the error text does appear in the log.
  • /sync-fork/orders matches on exact order.spec.githubRepository === repositoryUrl. REPO_URL is https://github.com/${{ github.repository }}, which has no .git and a case taken from the repo name. The PR says it captured a live dev response for this form. Worth confirming for each of the six repos in Eyevinn/osaas-maker-manager#255.
  • If orderProvider were not wired in the maker-manager deployment, the route would return [] silently. The route code treats that as the no-orders case, and the dedicated key's POST would then 403. #255 covers this.

Escape Hatch Coverage

  • The Expected at most one order guard and the missing-secret guard are workflow-level failures, not request-handler rejections. The PR's harness covers both the single-order passing path and the failure cases. No bypass is claimed. Fine.

Object Shape Verification

  • No spread or re-serialisation of fetched objects in the diff.

Contract diff

  • No request handler or timeout changed. Files checked: .github/workflows/sync-fork-dev.yml only. The 20s polling and 60/120 loop counts are unchanged. This is a CI client, not a server handler.

Impact Analysis

  • Blast radius: dev sync-fork workflow in python-runner only; the same file is said to be byte-identical across six runner repos (sibling PRs are separate).
  • Downstream consumers: none in code.
  • Coordinated changes needed: the dev sync-fork-api-key Secret, the maker-manager restart, and the GitHub secret OSC_DEV_SYNC_FORK_API_KEY must exist before or soon after merge. The PR states this, and says a run before then fails at the missing-secret check.
  • Breaking change risk: low, and deliberate; the workflow fails closed with no fallback to the platform key.

Risks Not Tested

  • No live run against dev with the narrow key. The harness uses a fake curl, and actionlint and shellcheck were not run. I did not execute the harness myself.
  • The API_KEY_SYNC_FORK deployment state on dev (Secret key present, pod restarted) could not be checked from here.
  • I could not clone osaas-maker-manager locally (403), so I read the route source through the GitHub API on main.

Research Suggestions

  • Confirm in #255 that a real run succeeds with the narrow key before deleting OSC_DEV_MAKER_API_KEY.

Verdict

No blocking issues found.


Reviewed by PR Research Validator. This review checks whether code is based on verified sources rather than assumptions.

@eyevinn-developer
eyevinn-developer Bot merged commit efe088b into main Nov 1, 2026
@eyevinn-developer
eyevinn-developer Bot deleted the ci-sync-fork-dev-narrow-key branch November 1, 2026 20:48
eyevinn-developer Bot pushed a commit that referenced this pull request Nov 2, 2026
…order lookup (#31)" (#32)

Co-authored-by: Claude <noreply@anthropic.com>
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.

2 participants