Skip to content

scripts: harden sftp.test - #1206

Open
ejohnstown wants to merge 1 commit into
wolfSSL:masterfrom
ejohnstown:ccb-phase1-2
Open

scripts: harden sftp.test#1206
ejohnstown wants to merge 1 commit into
wolfSSL:masterfrom
ejohnstown:ccb-phase1-2

Conversation

@ejohnstown

@ejohnstown ejohnstown commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

The ready-file counter is shared by every create_port call and was never reset, so the twenty iterations are a budget for the whole run rather than per server start. A slow first start leaves later ones with no wait at all, and the test fails with "NO ready file" instead of waiting. The guard after the loop then disagreed with the loop as well: the wait is for a non-empty file, but the guard only asked whether the file existed, so a wait that ran out still went on to read an empty port.

  • reset the counter in create_port, so each server start gets the full wait and a new call site cannot forget it
  • test the guard on -s, matching what the loop waited for
  • drop echo -e, undefined for POSIX sh and printed literally by shells that do not take the flag; the escapes were only ever blank lines
  • quote the expansions, use $(...) instead of backticks, and grep -q
  • exit 1 rather than exit -1 from the trap handler
  • drop the redundant PWD assignment before the set-directory test, and the second copy of the wolfsftp executable check

Leaves shellcheck -s sh clean apart from SC2329 on the trap handler.

Copilot AI lite review requested due to automatic review settings August 27, 2026 23:12

Copilot AI left a comment

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.

Pull request overview

This PR hardens the scripts/sftp.test POSIX sh test runner to reduce flakiness around server startup/ready-file handling and to improve portability and robustness of the shell code.

Changes:

  • Reset the ready-file wait counter per create_port invocation and align the post-loop guard with the loop’s -s condition.
  • Remove non-POSIX echo -e usage and apply safer shell practices (quoting, $(...), grep -q, exit 1 in trap).
  • Simplify redundant checks/assignments in the test flow.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread scripts/sftp.test
@ejohnstown
ejohnstown requested review from wolfSSL-Fenrir-bot and removed request for wolfSSL-Fenrir-bot August 27, 2026 23:25
The ready-file counter is shared by every create_port call and was never
reset, so the twenty iterations are a budget for the whole run rather
than per server start. A slow first start leaves later ones with no wait
at all, and the test fails with "NO ready file" instead of waiting. The
guard after the loop then disagreed with the loop as well: the wait is
for a non-empty file, but the guard only asked whether the file existed,
so a wait that ran out still went on to read an empty port.

- reset the counter in create_port, so each server start gets the full
  wait and a new call site cannot forget it
- test the guard on -s, matching what the loop waited for
- drop "echo -e", undefined for POSIX sh and printed literally by shells
  that do not take the flag; the escapes were only ever blank lines
- quote the expansions, use $(...) instead of backticks, and grep -q
- exit 1 rather than exit -1 from the trap handler
- drop the redundant PWD assignment before the set-directory test, and
  the second copy of the wolfsftp executable check

Leaves shellcheck -s sh clean apart from SC2329 on the trap handler.

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-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.

Fenrir Automated Review — PR #1206

No scan targets match the changed files in this PR. Review skipped.

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.

3 participants