Skip to content

fix(docs-noob-tester): replace STATUS=$(curl) with until curl to survive set -e #28624

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 2 commits into from
Apr 26, 2026
+32 −18
Merged

fix(docs-noob-tester): replace STATUS=$(curl) with until curl to survive set -e #28624

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
831d0c1
Initial plan
Copilot Apr 26, 2026
78eecb3
fix: use until curl pattern in docs-noob-tester server readiness check
Copilot Apr 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions .github/workflows/docs-noob-tester.lock.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 18 additions & 4 deletions .github/workflows/docs-noob-tester.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,25 @@ pre-agent-steps:
echo "Server PID: $PID"
- name: Wait for server readiness
run: |
for i in {1..45}; do # 45 attempts × 3s = 135s max wait
STATUS=$(curl -s -o /dev/null -w "%{http_code}" http://localhost:4321/gh-aw/)
[ "$STATUS" = "200" ] && echo "Server ready at http://localhost:4321/gh-aw/!" && break
echo "Waiting for server... ($i/45) (status: $STATUS)" && sleep 3
MAX_WAIT=135 # 45 attempts × 3s = 135s max wait
WAITED=0
Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

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

This workflow imports shared/docs-server-lifecycle.md, but that shared document’s “Waiting for Server Readiness” section still shows the old STATUS=$(curl ...) pattern (shared/docs-server-lifecycle.md:54-58). Consider updating the shared instructions (or noting the newer pattern here) to avoid the prompt/documentation drifting from the actual pre-agent step behavior.

Suggested change
WAITED=0
WAITED=0
# Note: shared/docs-server-lifecycle.md may still show the older
# STATUS=$(curl ...) example, but this workflow intentionally uses
# the newer direct until-curl pattern plus PID/timeout checks so the
# documented behavior here matches the actual pre-agent step.

Copilot uses AI. Check for mistakes.
until curl -sf http://localhost:4321/gh-aw/ > /dev/null 2>&1; do
Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

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

curl -sf in the until condition has no connect/overall timeout, so a stalled connect/read can block the loop longer than the intended 3s cadence and make the effective wait exceed (or behave inconsistently with) MAX_WAIT. Consider adding --connect-timeout/--max-time (or similar) so each probe is bounded and the MAX_WAIT semantics stay reliable.

Suggested change
until curl -sf http://localhost:4321/gh-aw/ > /dev/null 2>&1; do
until curl -sf --connect-timeout 1 --max-time 2 http://localhost:4321/gh-aw/ > /dev/null 2>&1; do

Copilot uses AI. Check for mistakes.
# Check if the server process has already died
if [ -f /tmp/server.pid ] && ! kill -0 "$(cat /tmp/server.pid)" 2>/dev/null; then
echo "::error::Documentation server process died before becoming ready. Server log:"
cat /tmp/preview.log
exit 1
Comment on lines +57 to +60
Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

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

The crash-detection check uses kill -0 "$(cat /tmp/server.pid)" which is vulnerable to races/invalid contents (e.g., empty/partial PID file) and reintroduces a command substitution in a set -e script. Safer is to read the PID with shell redirection (read -r pid < /tmp/server.pid) and validate it’s a non-empty numeric PID before calling kill -0.

Suggested change
if [ -f /tmp/server.pid ] && ! kill -0 "$(cat /tmp/server.pid)" 2>/dev/null; then
echo "::error::Documentation server process died before becoming ready. Server log:"
cat /tmp/preview.log
exit 1
if [ -f /tmp/server.pid ]; then
read -r pid < /tmp/server.pid || pid=
case "$pid" in
''|*[!0-9]*)
echo "::error::Documentation server PID file is invalid before readiness check. Server log:"
cat /tmp/preview.log
exit 1
;;
esac
if ! kill -0 "$pid" 2>/dev/null; then
echo "::error::Documentation server process died before becoming ready. Server log:"
cat /tmp/preview.log
exit 1
fi

Copilot uses AI. Check for mistakes.
fi
WAITED=$((WAITED + 3))
if [ $WAITED -ge $MAX_WAIT ]; then
echo "::error::Documentation server did not start after ${MAX_WAIT}s. Server log:"
cat /tmp/preview.log
exit 1
fi
echo "Waiting for server... ($WAITED/${MAX_WAIT}s)"
sleep 3
done
echo "Server ready at http://localhost:4321/gh-aw/!"
- name: Detect bridge IP and write server URL
run: |
SERVER_IP=$(ip -4 route get 1.1.1.1 2>/dev/null | awk '{print $7; exit}')
Expand Down
Loading