.github/workflows/aws-gpu-tests.yml (1)

238-255: Cleanup job may skip termination if the test job fails early.

needs.run-unit-tests-on-aws.outputs.instance_id is only populated when the whole job succeeds.
If any later step (artifact download, Codecov upload) fails, the output is blank → instance leaks.

Safer pattern:

-    needs: run-unit-tests-on-aws
-    if: github.repository == 'newton-physics/newton' && always()
+    needs: run-unit-tests-on-aws
+    if: always()  # always run, independent of previous job result

and pass the ID via an artefact or workflow-level environment file ($GITHUB_ENV) written in the launch step and picked up here:

- if: needs.run-unit-tests-on-aws.outputs.instance_id != ''
+ env:
+   INSTANCE_ID: ${{ needs.run-unit-tests-on-aws.outputs.instance_id || env.INSTANCE_ID }}
  ...
- aws ec2 terminate-instances --instance-ids ${{ needs.run-unit-tests-on-aws.outputs.instance_id }}
+ aws ec2 terminate-instances --instance-ids "$INSTANCE_ID"

Ensures termination under every failure mode.

.github/workflows/aws-gpu-tests.yml (2)

160-178: Trailing spaces break YAML-lint and make reviews noisy.

Lines 166 and 174 carry stray spaces → yamllint error.
Quick fix:

-            echo "Current status: $STATUS. Waited $elapsed_time seconds..."␠
+            echo "Current status: $STATUS. Waited $elapsed_time seconds..."

[to delete the two offending spaces]

---

136-138: jq munging a full script into a single array element is brittle.

AWS-RunShellScript expects an array of commands. Embedding the entire multi-line script as one string works only if the remote interpreter honours embedded newlines; quoting errors silently turn the script into a single long line.

Simpler & safer:

- jq -n --arg script_body "$(cat remote_script.sh)" \
- '{ "commands": [$script_body] }' > ssm_params.json
+ aws ssm send-command \
+   --instance-ids "$INSTANCE_ID" \
+   --document-name "AWS-RunShellScript" \
+   --comment "Running Newton GPU unit tests" \
+   --parameters "commands=$(cat remote_script.sh | base64)" \
+   --cli-binary-format raw-in-base64-out \
+   ...

(or upload the script to S3 and execute bash /tmp/script.sh). Reduces JSON-escaping headaches.

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

.github/workflows/aws-gpu-tests.yml (2)

10-11: AMI filter pattern is still fragile – refine or pin to a stable AMI.
Wildcard sequence ???????? is brittle; the comment from the previous review remains valid. Use a build-pattern that will not break on the next AMI release or pin to an explicit owner alias instead.

---

125-129: Shell-injection risk: branch name is still interpolated directly.
BRANCH_NAME="${{ github.head_ref || github.ref_name }}" passes attacker-controlled data straight into the remote shell. The earlier mitigation proposal (use a sanitized env var) hasn’t been applied.

-          BRANCH_NAME="${{ github.head_ref || github.ref_name }}"
+          BRANCH_NAME="${BRANCH_NAME_ENV}"

and add at the step level:

env:
  BRANCH_NAME_ENV: ${{ github.event.pull_request.head.ref || github.ref_name }}

.github/workflows/aws-gpu-tests.yml (1)

172-190: Trailing whitespace triggers YAML-lint failures.
Lines 178 and 186 have stray spaces → yamllint error. Strip them to keep the CI green.

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

.github/workflows/aws-gpu-tests.yml (2)

154-188: final_status still misses edge cases like Cancelled or DeliveryTimedOut.

The catch-all branch sets final_status="$STATUS" and breaks, but STATUS can be values that are not actionable failures (Cancelled,TimedOut,Undelivered, …).
Recommend enumerating all success statuses and treat everything else as failure, e.g.

if [[ "$STATUS" == "Success" || "$STATUS" == "Cancelled" ]]; then
  :
elif [[ "$STATUS" == "InProgress" || "$STATUS" == "Pending" ]]; then
  ...
else
  final_status="$STATUS"
  break
fi

(or invert the condition: if [[ "$STATUS" != "InProgress" && "$STATUS" != "Pending" ]]; then … fi) to avoid silent masking.

---

160-179: Trailing whitespace flagged by YAMLlint

YAMLlint reports trailing spaces on lines 166 and 179 – minor, but fixing avoids noisy lint failures.

-              --output text)␠
+              --output text)
...
-            echo "Current status: $STATUS. Waited $elapsed_time seconds..."␠
+            echo "Current status: $STATUS. Waited $elapsed_time seconds..."

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

.github/workflows/aws-gpu-tests.yml (1)

88-96: Here-doc still unquoted → variables can expand on the runner side

The delimiter << EOF permits interpolation of all $FOO, $( … ) and back-ticks before the file is even copied to the EC2 box. A malicious value that slipped past safe_branch or any future variable introduced in the runner’s env would be executed locally with full GitHub-host permissions.

Quoting the delimiter eliminates this class of bugs with zero downside:

-          cat << EOF > remote_script.sh
+          cat <<'EOF' > remote_script.sh

Everything that really must be substituted (e.g. S3 bucket, key) can be injected explicitly after the file is generated or passed via SSM parameters/environment.

Also applies to: 120-123

.github/workflows/aws-gpu-tests.yml (3)

14-17: pull_request_target + AWS role = high-risk surface

Running untrusted PR code with an assumed AWS role (even if scoped) can expose the AWS account. A safer pattern is:

• Trigger on pull_request (not *_target) so the fork’s workflow file must reference the role – impossible.
• Or keep pull_request_target but checkout the merge commit instead of the attacker’s branch and pass --depth 1.

Please re-evaluate the threat model before merging.

Also applies to: 30-32

---

74-76: Possible job timeout before SSM command finishes

The job has a 75-minute timeout but:

1. EC2 boot (wait instance-status-ok) can take >10 min on bursty regions.
2. The polling loop waits up to 60 min for SSM.
3. Tests themselves may run 30 min on a cold GPU AMI.

A narrow margin (<5 min) remains. Either shorten TIMEOUT, increase timeout-minutes, or push polling into a separate job so cleanup never races the watchdog.

Also applies to: 160-195

---

172-173: Trailing whitespace – YAMLlint error

Lines 172 and 185 contain stray spaces that fail the default yamllint ruleset.

-              --output text)␠
+              --output text)
...
-              final_status="$STATUS"␠
+              final_status="$STATUS"

Minor, but it will break pipelines that enforce linting.

Also applies to: 185-186

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

.github/workflows/aws-gpu-tests.yml (1)

84-120: Remove double-quoting of an already printf %q-escaped branch name

printf '%q' wraps the value in single-quotes when needed.
Embedding that string again inside double-quotes (BRANCH_NAME="${safe_branch}") makes the quotes part of the value, so the remote git clone --branch looks for a ref literally called 'feature-xyz' and fails.

- safe_branch=$(printf '%q' "${BRANCH_NAME_ENV}")
- ...
- BRANCH_NAME="${safe_branch}"
+safe_branch=$(printf '%q' -- "${BRANCH_NAME_ENV}")   # keep existing escaping
+# no extra quotes – value is already shell-safe
+BRANCH_NAME=${safe_branch}

.github/workflows/aws-gpu-tests.yml (2)

18-24: Cancellation can orphan EC2 instances – put termination in an independent cleanup job

When a newer run hits the same concurrency group, GitHub cancels the entire previous job; step-level if: always() hooks never fire, so aws ec2 terminate-instances is skipped and the GPU instance leaks.

Create a separate cleanup job that

• has if: always()
• is not in the concurrency group (omit needs: or use a different group key)
• fetches the INSTANCE_ID artifact (or tags) and terminates the instance.

This guarantees the teardown runs even after a workflow-level cancellation.

Also applies to: 247-255

---

164-186: Minor: trailing whitespace trips yamllint

Static analysis already reports trailing spaces on lines 170 & 183.
Strip them to keep the workflow lint-clean.

-          while [ $elapsed_time -lt $TIMEOUT ]; do␠
+          while [ $elapsed_time -lt $TIMEOUT ]; do
...
-            echo "Current status: $STATUS. Waited $elapsed_time seconds..."␠
+            echo "Current status: $STATUS. Waited $elapsed_time seconds..."

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

Learnt from: shi-eric
PR: newton-physics/newton#386
File: .github/workflows/aws-gpu-tests.yml:86-87
Timestamp: 2025-07-12T17:40:10.078Z
Learning: Git does not allow spaces in branch names. Branch names with spaces will result in "is not a valid branch name" errors.

.github/workflows/aws-gpu-tests.yml (4)

90-96: Redundant re-quoting may leak literal quote characters into $BRANCH_NAME

printf '%q' already returns a shell-escaped token. Wrapping it again in double quotes (line 128) can embed the quotes themselves when no escaping is needed, yielding git clone --branch "'feature-x'" …. Consider dropping the extra double quotes:

-SAFE_BRANCH=$(printf '%q' "${BRANCH_NAME_ENV}")
+SAFE_BRANCH=$(printf '%q' -- "${BRANCH_NAME_ENV}")   # -- guards against leading ‘-’

…

-BRANCH_NAME="${SAFE_BRANCH}"
+BRANCH_NAME=${SAFE_BRANCH}

Minor, but removes one layer of quoting and avoids surprises.

---

50-61: AMI lookup is region-hard-coded – breaks in non-us-east-2 regions

The SSM parameter path is valid only where AWS publishes that specific DLAMI build. If the repository later sets AWS_REGION to another region (or contributors fork with a different default), get-parameter returns ParameterNotFound and the job aborts.

Recommend either:

1. Pin the AMI ID in the workflow inputs (cheap, deterministic), or
2. Drive the parameter path from the region:

PARAM="/aws/service/deeplearning/ami/x86_64/base-oss-nvidia-driver-gpu-ubuntu-24.04/${{ env.AWS_REGION }}/latest/ami-id"
LATEST_AMI_ID=$(aws ssm get-parameter --region "${{ env.AWS_REGION }}" \
              --name "$PARAM" --query 'Parameter.Value' --output text)

---

150-152: Risk of exceeding SSM 16 KB payload limit

$(cat remote_script.sh) inlines the entire script into a single JSON string. Once the script grows beyond ~16 KB the send-command call is rejected.

Mitigations:

• gzip → base64 the script and decode on the instance, or
• store the script in S3 and reference it via --document-version / --parameters sourceType=S3 ….

Nice-to-have now, but future-proofs the workflow.

---

173-196: Trailing whitespace flagged by yamllint

Lines 179 and 192 contain stray spaces that yamllint reports. They’re inside a shell heredoc, so harmless at runtime, but cleaning them avoids noisy CI lint failures.

-            echo "Current status: $STATUS. Waited $elapsed_time seconds..."␠
+            echo "Current status: $STATUS. Waited $elapsed_time seconds..."

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

Learnt from: shi-eric
PR: newton-physics/newton#386
File: .github/workflows/aws-gpu-tests.yml:86-87
Timestamp: 2025-07-12T17:40:10.078Z
Learning: Git does not allow spaces in branch names. Branch names with spaces will result in "is not a valid branch name" errors.

.github/workflows/aws-gpu-tests.yml (1)

182-195: Trailing whitespace – fails yamllint

yamllint flags lines 182 and 195 for trailing spaces.
They are inside a bash heredoc, so removal is safe and keeps the CI green.

-            
+

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

Learnt from: shi-eric
PR: newton-physics/newton#386
File: .github/workflows/aws-gpu-tests.yml:86-87
Timestamp: 2025-07-12T17:40:10.078Z
Learning: Git does not allow spaces in branch names. Branch names with spaces will result in "is not a valid branch name" errors.

.github/workflows/aws-gpu-tests.yml (1)

126-129: Forked-PR code is never executed – clone URL & ref still point at base repository

REPO_URL and BRANCH_NAME are hard-wired to the target repo / branch.
When somebody opens a PR from a fork, the branch of the contributor does not exist in github.repository; the workflow therefore tests stale code and can report false-green.

-REPO_URL="https://github.com/${{ github.repository }}.git"
-BRANCH_NAME="${SAFE_BRANCH}"
+if [[ "${{ github.event.pull_request.head.repo.full_name }}" != "${{ github.repository }}" ]]; then
+  # External fork – test the exact commit under review
+  REPO_URL="${{ github.event.pull_request.head.repo.clone_url }}"
+  BRANCH_NAME="${{ github.event.pull_request.head.sha }}"
+else
+  REPO_URL="https://github.com/${{ github.repository }}.git"
+  BRANCH_NAME="${SAFE_BRANCH}"
+fi

Without this, GPU tests give a misleading sense of safety for every external contribution.

.github/workflows/aws-gpu-tests.yml (2)

90-96: printf %q adds complexity for little gain

SAFE_BRANCH=$(printf '%q' …) produces shell-escaped fragments like 'feature/xyz'.
Because you later embed the value in an assignment (BRANCH_NAME=${SAFE_BRANCH}) the extra quoting is stripped anyway for normal branch names, while exotic names (: ~ etc.) are already rejected by Git. Dropping this makes the script easier to read:

-SAFE_BRANCH=$(printf '%q' "${BRANCH_NAME_ENV}")
+SAFE_BRANCH="${BRANCH_NAME_ENV}"

---

180-195: Trailing whitespace flagged by yamllint

Lines 181 and 194 contain stray spaces which fail the lint step. Quick tidy-up:

-              --output text)␠
+              --output text)
…
-              echo "SSM command failed or returned unexpected status: $STATUS"␠
+              echo "SSM command failed or returned unexpected status: $STATUS"

Pure style, but keeps CI green.

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

Learnt from: shi-eric
PR: newton-physics/newton#386
File: .github/workflows/aws-gpu-tests.yml:86-87
Timestamp: 2025-07-12T17:40:10.078Z
Learning: Git does not allow spaces in branch names. Branch names with spaces will result in "is not a valid branch name" errors.

.github/workflows/aws-gpu-tests.yml (4)

100-113: printf %q + double-quoting still injects quotes into the remote variable

SAFE_BRANCH may already be quoted ('feature/xyz').
Assigning it again inside double quotes

BRANCH_NAME="${SAFE_BRANCH}"

preserves the literal single quotes and yields a branch called 'feature/xyz' on the remote host. git clone --branch will fail.

Either:

-BRANCH_NAME="${SAFE_BRANCH}"
+BRANCH_NAME=${SAFE_BRANCH}   # no extra layer of quoting

or drop printf %q altogether and rely on Git’s own ref rules.

---

165-167: SSM payload can exceed the 16 KB limit – externalise the script

send-command embeds the entire remote_script.sh as a single JSON element.
SSM RunShellScript rejects payloads > 16 KB with ValidationException, and this script is already sizeable.

Recommended pattern:

1. aws s3 cp remote_script.sh s3://$S3_BUCKET/$S3_KEY/remote_script.sh
2. Send a tiny SSM command: aws s3 cp … && bash remote_script.sh

Keeps the payload small and future-proofs the workflow.

---

275-283: Instance-leak on concurrency cancellation persists

The termination step lives inside the same job that can be cancelled by
concurrency.cancel-in-progress.
When a new push arrives, the running job is aborted before reaching this
step and the GPU instance keeps running.

Move the cleanup to a separate cleanup job with if: always() and no needs
dependency, pass the INSTANCE_ID via an output or artifact, and terminate the
instance there.

---

144-148: Run the tests against the PR’s code, not the base repository

For PRs coming from forks https://github.com/${{ github.repository }}.git does not contain the contributor’s commits.
Result: the workflow happily green-lights stale code.

Quick fix:

-REPO_URL="https://github.com/${{ github.repository }}.git"
-BRANCH_NAME="${SAFE_BRANCH}"
+if [[ -n "${{ github.event.pull_request.head.repo.clone_url }}" ]]; then
+  REPO_URL="${{ github.event.pull_request.head.repo.clone_url }}"
+  BRANCH_NAME="${{ github.event.pull_request.head.ref }}"
+else
+  REPO_URL="https://github.com/${{ github.repository }}.git"
+  BRANCH_NAME=${SAFE_BRANCH}
+fi

This clones the exact commit under review while still covering internal pushes.

.github/workflows/aws-gpu-tests.yml (1)

73-75: Avoid embedding literal single-quotes in the GitHub-Repository tag

TAGS="$TAGS,{Key=GitHub-Repository,Value='${{ github.repository }}'}"
The inner single quotes become part of the tag value ('newton-physics/newton').
Drop them:

-TAGS="$TAGS,{Key=GitHub-Repository,Value='${{ github.repository }}'}"
+TAGS="$TAGS,{Key=GitHub-Repository,Value=${{ github.repository }}}"

Minor, but keeps tag values clean and easier to query.

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

Learnt from: shi-eric
PR: newton-physics/newton#386
File: .github/workflows/aws-gpu-tests.yml:86-87
Timestamp: 2025-07-12T17:40:10.078Z
Learning: Git does not allow spaces in branch names. Branch names with spaces will result in "is not a valid branch name" errors.

.github/PULL_REQUEST_TEMPLATE.md (1)

25-25: Checklist item is clear – consider explicitly mentioning GPU tests

You removed the outdated “GitHub does not perform GPU testing” disclaimer, which is great.
Since GPU tests are now executed by the new AWS workflow, you might add a short hint such as “GPU tests pass in CI” so contributors are aware this box covers both CPU and GPU paths.

.github/workflows/aws-gpu-tests.yml (2)

63-72: Tag value contains literal single-quotes

GitHub-Repository is assembled as
Value='${{ github.repository }}' which results in the tag value literally including the ' characters ('newton-physics/newton').
AWS accepts it, but it bloats searches/filters and looks odd in the console.

-TAGS="$TAGS,{Key=GitHub-Repository,Value='${{ github.repository }}'}"
+TAGS="$TAGS,{Key=GitHub-Repository,Value=${{ github.repository }}}"

---

108-112: Harden the remote script with set -u

set -e and set -o pipefail are enabled, but missing set -u means unset variables silently expand to empty strings (e.g., if $PR_NUMBER or $S3_BUCKET were ever unset).
Adding it improves safety with negligible risk.

-set -e
-set -o pipefail
+set -euo pipefail

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

Learnt from: shi-eric
PR: newton-physics/newton#386
File: .github/workflows/aws-gpu-tests.yml:144-156
Timestamp: 2025-07-12T19:46:06.538Z
Learning: In the newton-physics/newton repository workflow discussions, the maintainer shi-eric prefers that forked PR testing behavior is intentionally designed to fail rather than test untrusted code from forks on AWS infrastructure.

Learnt from: shi-eric
PR: newton-physics/newton#386
File: .github/workflows/aws-gpu-tests.yml:65-82
Timestamp: 2025-07-12T19:46:47.510Z
Learning: In the newton-physics/newton repository AWS GPU workflow, the workflow only triggers on `pull_request_target` and `push` events, not on `pull_request` events. The `pull_request_target` event is used for testing PRs from forks safely on AWS infrastructure.

Learnt from: shi-eric
PR: newton-physics/newton#386
File: .github/workflows/aws-gpu-tests.yml:275-283
Timestamp: 2025-07-12T19:45:04.640Z
Learning: In GitHub Actions, the `if: always()` condition runs even when a job is cancelled due to concurrency settings with `cancel-in-progress: true`. This means cleanup steps with `if: always()` will execute properly to prevent resource leaks, even if the main workflow is cancelled by a newer run.

Learnt from: shi-eric
PR: newton-physics/newton#386
File: .github/workflows/aws-gpu-tests.yml:65-82
Timestamp: 2025-07-12T19:46:47.510Z
Learning: In the newton-physics/newton repository AWS GPU workflow, the workflow only triggers on `pull_request_target` and `push` events, not on `pull_request` events. The `pull_request_target` event is used for testing PRs from forks safely on AWS infrastructure.

Learnt from: shi-eric
PR: newton-physics/newton#386
File: .github/workflows/aws-gpu-tests.yml:144-156
Timestamp: 2025-07-12T19:46:06.538Z
Learning: In the newton-physics/newton repository workflow discussions, the maintainer shi-eric prefers that forked PR testing behavior is intentionally designed to fail rather than test untrusted code from forks on AWS infrastructure.

Learnt from: shi-eric
PR: newton-physics/newton#386
File: .github/workflows/aws-gpu-tests.yml:65-82
Timestamp: 2025-07-12T19:46:47.510Z
Learning: In the newton-physics/newton repository AWS GPU workflow, the workflow only triggers on `pull_request_target` and `push` events, not on `pull_request` events. The `pull_request_target` event is used for testing PRs from forks safely on AWS infrastructure.

Learnt from: shi-eric
PR: newton-physics/newton#386
File: .github/workflows/aws-gpu-tests.yml:144-156
Timestamp: 2025-07-12T19:46:06.538Z
Learning: In the newton-physics/newton repository workflow discussions, the maintainer shi-eric prefers that forked PR testing behavior is intentionally designed to fail rather than test untrusted code from forks on AWS infrastructure.

Learnt from: shi-eric
PR: newton-physics/newton#386
File: .github/workflows/aws-gpu-tests.yml:275-283
Timestamp: 2025-07-12T19:45:04.640Z
Learning: In GitHub Actions, steps with `if: always()` continue to run even when the workflow is canceled via `concurrency.cancel-in-progress: true`. GitHub Actions re-evaluates if conditions during cancellation, and since `if: always()` always evaluates to true, those steps are not canceled and will continue to execute. This means cleanup steps with `if: always()` will run properly to prevent resource leaks even when workflows are cancelled due to concurrency limits.

Learnt from: shi-eric
PR: newton-physics/newton#386
File: .github/workflows/aws-gpu-tests.yml:275-283
Timestamp: 2025-07-12T19:45:04.640Z
Learning: In GitHub Actions, the `if: always()` condition runs even when a job is cancelled due to concurrency settings with `cancel-in-progress: true`. This means cleanup steps with `if: always()` will execute properly to prevent resource leaks, even if the main workflow is cancelled by a newer run.

Learnt from: shi-eric
PR: newton-physics/newton#386
File: .github/workflows/aws-gpu-tests.yml:0-0
Timestamp: 2025-07-12T19:46:37.456Z
Learning: For AWS SSM send-command payload limits, always measure the actual script size before suggesting optimizations. The 16KB limit is rarely hit by typical GitHub Actions workflow scripts, which are usually only a few KB in size.

Learnt from: shi-eric
PR: newton-physics/newton#386
File: .github/workflows/aws-gpu-tests.yml:86-87
Timestamp: 2025-07-12T17:40:10.078Z
Learning: Git does not allow spaces in branch names. Branch names with spaces will result in "is not a valid branch name" errors.