Skip to content

PET: Remove duplicated code#1077

Merged
bkchr merged 6 commits into
mainfrom
bkchr-remove-duplicates-pet
Feb 24, 2026
Merged

PET: Remove duplicated code#1077
bkchr merged 6 commits into
mainfrom
bkchr-remove-duplicates-pet

Conversation

@bkchr

@bkchr bkchr commented Feb 19, 2026

Copy link
Copy Markdown
Contributor
  • Does not require a CHANGELOG entry

@bkchr bkchr mentioned this pull request Feb 19, 2026
5 tasks

@github-actions github-actions 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.

Found a few robustness issues in the GitHub Actions workflow around endpoint lookup and template substitution that can lead to invalid YAML or silently starting Subway with no endpoints.

Comment on lines +410 to +417
CHAIN_NAME=$(echo "$chain" | jq -r '.chain')
PORT=$(echo "$chain" | jq -r '.port')

# Get endpoints from runtimes-matrix.json
ENDPOINTS=$(jq -r --arg chain "$CHAIN_NAME" '.[] | select(.name == $chain) | .uris | map("\"" + . + "\"") | join(", ")' .github/workflows/runtimes-matrix.json)

echo "Starting Subway for $CONFIG on port $PORT..."
sed "s/{{PORT}}/$PORT/g" .github/subway-configs/$CONFIG > /tmp/subway-$PORT.yml
echo "Starting Subway for $CHAIN_NAME on port $PORT..."
sed -e "s/{{PORT}}/$PORT/g" -e "s|{{ENDPOINTS}}|$ENDPOINTS|g" .github/subway-configs/template.yml > /tmp/subway-$PORT.yml

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The loop uses ... | while read chain; do (without -r) and then parses JSON from $chain. read will treat backslashes as escapes, which can corrupt JSON strings. Use while IFS= read -r chain; do to ensure the JSON line is read verbatim.

Comment thread .github/workflows/test.yml Outdated
echo "Starting Subway for $CONFIG on port $PORT..."
sed "s/{{PORT}}/$PORT/g" .github/subway-configs/$CONFIG > /tmp/subway-$PORT.yml
echo "Starting Subway for $CHAIN_NAME on port $PORT..."
sed -e "s/{{PORT}}/$PORT/g" -e "s|{{ENDPOINTS}}|$ENDPOINTS|g" .github/subway-configs/template.yml > /tmp/subway-$PORT.yml

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The sed replacement injects $ENDPOINTS directly into the replacement string. If any URI ever contains & (sed replacement expands & to the matched text) or the chosen delimiter (|), the generated YAML will be corrupted. Either escape the replacement (ENDPOINTS_ESCAPED=$(printf '%s' "$ENDPOINTS" | sed 's/[&|\\]/\\&/g')) before sed, or avoid sed entirely by generating the YAML with a templating-safe approach (e.g., have jq emit the full JSON array and substitute that).

Comment thread .github/subway-configs/template.yml Outdated
@github-actions

Copy link
Copy Markdown

Review required! Latest push from author must always be reviewed


echo "Starting Subway for $CONFIG on port $PORT..."
sed "s/{{PORT}}/$PORT/g" .github/subway-configs/$CONFIG > /tmp/subway-$PORT.yml
ENDPOINTS=$(jq -c --arg chain "$CHAIN_NAME" '.[] | select(.name == $chain) | .uris' .github/workflows/runtimes-matrix.json)

@rockbmb rockbmb Feb 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Huh. I must have prompted Claude incorrectly, if I wasn't able to find this file.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The file is here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, I know; what I meant was, while I worked in #1068, I didn't find it myself, and Claude didn't take it into account as we worked.

@rockbmb

rockbmb commented Feb 23, 2026

Copy link
Copy Markdown
Contributor

PET is failing because the runtime changes introduced in #1065 are incompatible with some tests.

#1078 made the check optional, so this can be merged with them failing; I will fix the PET branch being used in a future PR.

@bkchr bkchr enabled auto-merge (squash) February 23, 2026 12:26
@bkchr bkchr merged commit 015414e into main Feb 24, 2026
80 of 94 checks passed
@rockbmb

rockbmb commented Feb 24, 2026

Copy link
Copy Markdown
Contributor

I will fix the PET branch being used in a future PR.

#1087

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.

5 participants