Stabilize remote full-ci test startup paths#22742
Conversation
a422183 to
db00244
Compare
b66cd84 to
00ab833
Compare
db00244 to
a4e48cb
Compare
00ab833 to
086e533
Compare
a4e48cb to
21c7e73
Compare
086e533 to
0ca29ce
Compare
21c7e73 to
ca1d3de
Compare
224d9e0 to
230d335
Compare
ca1d3de to
098833a
Compare
230d335 to
73f5b5a
Compare
1a8be67 to
672e10b
Compare
73f5b5a to
3ea70c7
Compare
672e10b to
db8fe52
Compare
3ea70c7 to
4344033
Compare
db8fe52 to
77a8012
Compare
4344033 to
6736853
Compare
77a8012 to
c2a9217
Compare
6736853 to
d95d0bd
Compare
c2a9217 to
d118d31
Compare
7cfbea4 to
8f5a4cb
Compare
8c0690f to
8d80010
Compare
c85b53b to
5d5c4c7
Compare
8d80010 to
5b422b9
Compare
5d5c4c7 to
a4c3a1d
Compare
a4c3a1d to
2862410
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 094370503d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| container_name="${CODEX_TEST_REMOTE_ENV_CONTAINER_NAME:-codex-remote-test-env-local-$(date +%s)-${RANDOM}}" | ||
| codex_binary_path="${REPO_ROOT}/codex-rs/target/debug/codex" | ||
| remote_codex_path="${CODEX_TEST_REMOTE_CODEX_PATH:-/tmp/codex-remote-env/codex}" |
There was a problem hiding this comment.
Create the custom remote Codex directory
When CODEX_TEST_REMOTE_CODEX_PATH is set to a path outside /tmp/codex-remote-env, this assignment is accepted but setup_remote_env still only creates /tmp/codex-remote-env before docker cp. Docker does not create missing parent directories for a container destination, so a value like CODEX_TEST_REMOTE_CODEX_PATH=/opt/codex-test/codex fails before the remote tests start; create dirname "$remote_codex_path" in the container before copying/linking the binary.
Useful? React with 👍 / 👎.
|
|
||
| container_name="${CODEX_TEST_REMOTE_ENV_CONTAINER_NAME:-codex-remote-test-env-local-$(date +%s)-${RANDOM}}" | ||
| codex_binary_path="${REPO_ROOT}/codex-rs/target/debug/codex" | ||
| remote_codex_path="${CODEX_TEST_REMOTE_CODEX_PATH:-/tmp/codex-remote-env/codex}" |
There was a problem hiding this comment.
Require an installed Codex path for external exec servers
When this script is sourced with CODEX_TEST_REMOTE_EXEC_SERVER_URL already set, the copy/link block is skipped, but this new default is still exported and the test harness now uses it for config.codex_self_exe and config.codex_linux_sandbox_exe. In that external-server mode there is no guarantee that /tmp/codex-remote-env/codex exists on the remote host, so tests that re-enter Codex or sandbox filesystem helpers can fail with a missing helper path; only set/export this default when the script installs the binary, or require callers with a preexisting exec server to provide a valid remote path.
Useful? React with 👍 / 👎.
| requests: vec![vec![]], | ||
| response_headers: Vec::new(), | ||
| accept_delay: Some(Duration::from_millis(500)), | ||
| accept_delay: Some(Duration::from_secs(5)), |
There was a problem hiding this comment.
Keep the leak check longer than the sideband delay
With the sideband accept delay increased to 5 seconds, this test still only waits 700 ms before asserting that no stale event or handshake occurred and then shuts the fake websocket server down. If closing the conversation fails to abort the pending sideband connect, the leaked task will remain blocked in this delay and only surface after the 5-second accept completes, so the regression this test is meant to catch now passes; wait past the configured delay or use a shorter delay than the stale-event timeout.
Useful? React with 👍 / 👎.
c56801e to
cee926d
Compare
3ac6c21 to
69f17eb
Compare
d886ea2 to
bada872
Compare
059490a to
374d141
Compare
bada872 to
46835a9
Compare
6ac7958 to
445139c
Compare
445139c to
1714894
Compare
Why
After the earlier full-ci fixes, the Docker-backed remote test lane still exposed startup assumptions that do not hold in minimal remote containers: helper paths were inferred from local defaults and remote denial tests requested
bash.Failure evidence: remote Ubuntu test job with uploaded nextest JUnit artifact
What changed
docker cpsh -lcsh/non-login shell settings available in the minimal containerCI evidence
Recent failed
rust-ci-fulltest runs that exposed this flake family:Those failed test lanes uploaded nextest JUnit artifacts successfully. Representative artifacts from the latest run:
Walkthrough
bash.scripts/test-remote-env.shplus the remote unified-exec fixtures in this PR.sh/non-login shell settings, and keep the remote harness pointed at the binary the script actually installed.Verification
origin/main; GitHub now shows basemainand a one-commit standalone diff containing only the intended three files