Skip to content

ci(worker): guard strategy selection in Worker Operations CI#64

Merged
liobrasil merged 1 commit into
navid/new-worker-archfrom
codex/ci-strategy-guards-navid-unicommit
Feb 25, 2026
Merged

ci(worker): guard strategy selection in Worker Operations CI#64
liobrasil merged 1 commit into
navid/new-worker-archfrom
codex/ci-strategy-guards-navid-unicommit

Conversation

@liobrasil
Copy link
Copy Markdown
Contributor

@liobrasil liobrasil commented Feb 25, 2026

Summary

  • Add a Detect Strategy Identifier step in .github/workflows/worker_tests.yml before running worker tests.
  • In that step:
    • run flow scripts execute ./cadence/scripts/check_yieldvault_details.cdc 0x045a1763c93006ca
    • parse supportedStrategies
    • prefer an entry containing TracerStrategy, otherwise use the first supported entry
    • export it as STRATEGY_IDENTIFIER via $GITHUB_ENV
  • Update local/run_worker_tests.sh with a one-line change so STRATEGY_IDENTIFIER is read from env when present:
    • from hardcoded assignment
    • to ${STRATEGY_IDENTIFIER:-<previous-default>} fallback assignment

Why It Was Failing Before

  • Worker CI previously had no strategy-detection step.
  • run_worker_tests.sh always used the hardcoded value:
    • A.045a1763c93006ca.FlowYieldVaultsStrategies.TracerStrategy
  • In this emulator setup, the supported strategy was:
    • A.045a1763c93006ca.MockStrategies.TracerStrategy
  • Because of that mismatch, valid CREATE requests were accepted on EVM but dropped during Cadence-side processing.
  • Observable failure pattern in run 22385373935 / job 64794799651:
    • no vaults created for valid CREATE requests
    • pending queue drained to 0
    • refunds increased
    • tests 6, 13, and 27 failed

Why It Works Now

  • Worker CI now injects the actually supported strategy type into the environment before Run Worker Tests.
  • run_worker_tests.sh now picks that env value via shell fallback expansion, and uses it in its CREATE calls.
  • Net effect: CREATE requests use a supported strategy identifier, so they process successfully instead of being dropped.

Scope

  • Single commit.
  • Changes limited to Worker CI wiring + one script variable assignment fallback.
  • No contract logic changes.

@claude
Copy link
Copy Markdown

claude Bot commented Feb 25, 2026

PR Review: ci(worker): guard strategy selection in Worker Operations CI

Summary: This is a focused CI fix that correctly addresses the root cause of a failing Worker Operations test — a hardcoded strategy identifier that didn't match the one deployed in the emulator environment. The approach (runtime detection via the existing check_yieldvault_details.cdc script + env var injection with fallback) is sound.


Correctness & Logic

The core fix is correct. The mismatch between FlowYieldVaultsStrategies.TracerStrategy (hardcoded) and MockStrategies.TracerStrategy (actually deployed in the emulator) was clearly the cause of the failures described in the PR body. Detecting the strategy at runtime is the right approach.

The TracerStrategy-preference heuristic in the detection step is a reasonable tie-breaker when multiple strategies exist:

STRATEGY_IDENTIFIER=$(echo "$STRATEGY_LIST" | grep 'TracerStrategy' | head -n 1 || true)
if [ -z "$STRATEGY_IDENTIFIER" ]; then
  STRATEGY_IDENTIFIER=$(echo "$STRATEGY_LIST" | head -n 1)
fi

Concerns

1. Regex fragility — multi-line JSON output (medium)

SUPPORTED_STRATEGIES=$(echo "$YIELDVAULT_CHECK" | grep -oE '"supportedStrategies": \[[^]]*\]' || true)

[^]]* only matches characters that are not ], so this works only if the entire supportedStrategies array appears on a single line in the Flow CLI output. If the CLI ever pretty-prints the result across multiple lines (e.g., one strategy per line), this grep produces nothing and the step exits with ❌ Could not parse supported strategy list.

A safer alternative would be to use flow scripts execute ... --output json (if supported) and parse with jq, or at minimum collapse the output to a single line first:

YIELDVAULT_CHECK=$(flow scripts execute ... | tr -d '\n')

2. Fallback default in run_worker_tests.sh still points to the "wrong" identifier

STRATEGY_IDENTIFIER="${STRATEGY_IDENTIFIER:-A.045a1763c93006ca.FlowYieldVaultsStrategies.TracerStrategy}"

The fallback is the pre-fix hardcoded value (FlowYieldVaultsStrategies, not MockStrategies). This is fine for the CI path (env var is always injected), but local developers running run_worker_tests.sh directly without going through detect_strategy will still hit the original failure. A comment documenting this known limitation would help:

# NOTE: This fallback is for local development; CI injects STRATEGY_IDENTIFIER at runtime.
# If running locally, ensure this matches the strategy deployed in your emulator.
STRATEGY_IDENTIFIER="${STRATEGY_IDENTIFIER:-A.045a1763c93006ca.FlowYieldVaultsStrategies.TracerStrategy}"

3. GITHUB_ENV injection — minimal but worth noting

echo "STRATEGY_IDENTIFIER=$STRATEGY_IDENTIFIER" >> $GITHUB_ENV

If $STRATEGY_IDENTIFIER were to contain a newline (e.g., from a tampered emulator response), this would allow environment variable injection. In practice the risk is negligible here since the value comes from a locally running emulator and passes through grep | head -n 1, which prevents multi-line values. Still, using the heredoc delimiter syntax is the hardened pattern:

echo "STRATEGY_IDENTIFIER<<EOF" >> $GITHUB_ENV
echo "$STRATEGY_IDENTIFIER" >> $GITHUB_ENV
echo "EOF" >> $GITHUB_ENV

4. Step placed outside the # === RUN WORKER TESTS === section comment

Minor nit — the comment block # === RUN WORKER TESTS === appears after the new step, which makes it look like the strategy detection is infrastructure setup. Placing the comment above the detection step (or renaming it to # === DETECT RUNTIME CONFIG & RUN WORKER TESTS ===) would improve readability.


Positives

  • The detection step correctly echoes $YIELDVAULT_CHECK before parsing it, which will help future CI debugging.
  • Failing fast (exit 1) when the strategy list can't be parsed or is empty prevents silent test pollution.
  • The change is minimal and CI-only — no production contract changes.
  • The check_yieldvault_details.cdc script already exports supportedStrategies, so no additional Cadence changes were needed.

Summary

Area Status
Root cause fix ✅ Correct
Regex parsing robustness ⚠️ May fail on multi-line output
Local dev experience ⚠️ Fallback still broken for direct local runs
GITHUB_ENV injection hardening ℹ️ Low risk, but improvable
Code organization ℹ️ Minor comment placement nit

The fix is good and unblocks the CI. The regex concern is the most likely to cause a future recurrence and is worth addressing before merging.

@liobrasil liobrasil merged commit c717d13 into navid/new-worker-arch Feb 25, 2026
1 check passed
@liobrasil liobrasil deleted the codex/ci-strategy-guards-navid-unicommit branch February 25, 2026 16:19
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