Skip to content

sandcastle: proxy preflight can gate on stale sandcastle.config.json for pull_request_target workflows #317

Description

@arndvs

Migrated from arndvs/dotfiles-private#257 (sandcastle engine issues live on ctrlshft).

Parent PR: #255
Source comments:

Confidence: 25 = 50 +20 specific +15 concrete-alt-approaches −25 vague("Consider") −20 shared-util(proxy_preflight.sh used by 9 templates) −15 cross-file (same arithmetic for all 3 comments — identical root cause)

Interpretation

agent-update-branch.yml, agent-merge-pr.yml, and agent-fix-pr-feedback.yml all use pull_request_target, checkout {{DEFAULT_BRANCH}} for the "Checkout workflow actions" step, run proxy_preflight.sh against that checkout's sandcastle.config.json, and only later have sandcastle-setup check out the PR head SHA. If a PR modifies sandcastle.config.json (flips proxy, changes model), preflight's should_run decision reflects the base branch's config, not the PR's — causing false skips or false passes relative to what the subsequent agent run actually uses.

Verified this is scoped correctly: of the 9 agent-*.yml templates, only these 3 use pull_request_target with a base/head divergence; the other 6 (issue/dispatch-triggered) don't have this mismatch, so no other templates need the same fix.

Proposed approach

  • Add an optional config-path override to shft/templates/scripts/proxy_preflight.sh (e.g. SANDCASTLE_CONFIG_PATH env var) so it can read a config file from a location other than the checked-out workspace root.
  • In each of the 3 affected workflows, before the "Proxy preflight" step, fetch sandcastle.config.json from the PR head ref/SHA via gh api repos/${{ github.repository }}/contents/sandcastle.config.json?ref=${{ github.event.pull_request.head.sha }} into a temp file (avoids checking out untrusted PR code just to read one file), then set SANDCASTLE_CONFIG_PATH to that temp path for the preflight step.
  • Mirror the same change into .sandcastle/scripts/proxy_preflight.sh (installed-path copy) and regenerate via bin/update-sandcastle.sh / bin/init-sandcastle.sh so template and dogfood copies stay in sync.
  • Extend shft/templates/scripts/test_proxy_preflight.sh (+ .sandcastle mirror) with a case asserting the override path takes precedence over the workspace-root config.

Blockers / questions

  • Need to decide the exact gh api fallback behavior if the PR head's sandcastle.config.json was deleted (fall back to workspace config? fail closed?).
  • agent-merge-pr.yml and agent-fix-pr-feedback.yml gate on human-triggered label events (agent:merge, agent:fix) rather than raw pull_request_target opens, so the exact point to fetch the head config may differ slightly per workflow — needs a per-file pass, not one shared snippet.

Context for shft

Files to read:

  • shft/templates/scripts/proxy_preflight.sh — where the config-path override needs to be added
  • shft/templates/workflows/agent-update-branch.yml, agent-merge-pr.yml, agent-fix-pr-feedback.yml — the 3 affected workflows
  • bin/update-sandcastle.sh / bin/init-sandcastle.sh — mirror-sync logic for .sandcastle/*

Acceptance criteria:

  • proxy_preflight.sh supports reading config from an override path/env var without checking out untrusted PR code
  • All 3 affected workflows fetch and preflight against the PR head's sandcastle.config.json
  • .sandcastle/ mirror copies updated to match
  • New hermetic test case covers the override behavior

Feedback loops:

  • bash shft/templates/scripts/test_proxy_preflight.sh
  • bash .sandcastle/scripts/test_proxy_preflight.sh
  • npm test

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    source:architecture-reviewPRDs proposed by the automated architecture-review workflow

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions