Repository navigation
fix(tests): fix 8 shell suites for Ubuntu CI portability (closes #1961) - #1970
liususan091219 merged 7 commits into
Conversation
Coverage Gate✅ No Python changes — nothing to gate (bar: 95% on changed lines). |
qingyun-wu
left a comment
There was a problem hiding this comment.
LGTM (non-blocking notes). The stat ordering fix is the real root cause, not a band-aid: on GNU coreutils -f means --file-system, so BSD-first stat -f %m file treats %m as a filename, exits 1 and prints filesystem info to stdout — poisoning the arithmetic in codex-bounded.sh's watchdog and the inode compare in migration_safety_helpers.sh. Trying -c first and falling back to -f is correct on both platforms (BSD rejects -c cleanly to stderr). The python-based _perm/_mtime helpers in startup-env-token-persist.test.sh are a cleaner portable path than juggling stat syntaxes. Verified SUTANDO_TEST_MODE=1 is the sanctioned test-only escape hatch (sutando_config.py:314) that re-enables the SUTANDO_WORKSPACE override #1440 disabled — so the cron-gate test additions are right, not a workaround.
CI green (which is the whole point for a CI-portability PR).
Notes:
- This branch is stacked on #1969 — the
src/agent-api.py+agent-api-delegation.test.pychanges here are that PR's content, not new to #1970. Fine as a stack, just merge #1969 first so this diff shrinks to the shell-suite fixes it's actually about. - Still marked draft; flip to ready once the stack ordering is settled.
| # Defense-in-depth: apply the same id-shape gate as the submit side so | ||
| # both paths enforce the same invariant (#1959). _safe_path handles | ||
| # traversal, but valid_task_id additionally rejects unsupported charsets. | ||
| if not local_task_protocol.valid_task_id(stem): |
There was a problem hiding this comment.
This shell-test PR is currently carrying the #1969 agent-api behavior change as well (delegation_read_result now gates names through valid_task_id). That makes the PR broader than the stated six shell-suite fixes and can merge/close the delegation-read change out of order. Please rebase this branch after #1969 lands, or retarget it as an explicit stack so the diff only contains the shell portability/hermetic-env fixes.
There was a problem hiding this comment.
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-checked after the scope grew (now +80/-32, retitled "8 shell suites"). CI still green. Two scope notes since my earlier LGTM:
- It still carries #1969's
fix(agent-api): gate delegation_read_resultcommit (stacked) — merge #1969 first so this shrinks. - It now also includes
fix(sync-workspace): allow pre-wsId flat branch past init guard, which is a behavior change to sync-workspace, not a shell-test fix. That's a different concern from "fix N shell suites for Ubuntu CI" — worth splitting into its own PR (or retitling + calling it out in the description) so the sync-workspace change gets reviewed on its own merits rather than riding in on a test-portability PR. The test-portability commits themselves remain LGTM.
…i#1961 partial) startup-env-token-persist.test.sh used macOS-specific `stat -f '%Lp'` (permission mode) and `stat -f '%m'` (mtime) — both fail on GNU stat. Replace with `_perm()` / `_mtime()` Python helpers; 7/7 still pass locally. start-cli-claude-config-dir.test.sh had two CI failures: - ambient CLAUDE_CONFIG_DIR from the parent session bled into the helper-absent test, making it assert a non-empty value when empty was expected. Fix: `unset CLAUDE_CONFIG_DIR` in setup_sandbox. - workspace path mismatch: config used `${REPO_DIR}/workspace` while the test expected `$SUTANDO_WORKSPACE` (different dir), plus macOS symlink `/var` → `/private/var` produced path-prefix divergence. Fix: write the config with an absolute `$SANDBOX/workspace` path, and normalize the expected value via `python3 os.path.realpath`. 4/4 still pass locally. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QB3qZQrwponfum2JFNvHeh
…onichi#1961 more) GNU stat's -f flag means --file-system (boolean), not a format prefix. `stat -f '%d:%i' file` on Linux treats '%d:%i' as a filename arg — exits 1 but also writes default filesystem info to stdout, contaminating the output when the || fallback captures both streams. Result: _same_inode gets a multiline `a`/`b` value that doesn't match; _mtime gets a multiline `mt` that breaks `(( now - mt >= STALL ))` with "bad math expression", silently skipping the stall watchdog. Fix: try -c (GNU format flag) first, fall back to -f (BSD format flag). - src/migration_safety_helpers.sh: _same_inode swaps `-f`/`-c` order - skills/claude-codex/scripts/codex-bounded.sh: _mtime swaps `-f`/`-c` order Both test suites (migration-safety-helpers.test.sh, codex-bounded.test.sh) pass locally after this change. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QB3qZQrwponfum2JFNvHeh
…more) cron-gate.test.sh: add SUTANDO_TEST_MODE=1 alongside SUTANDO_WORKSPACE so the workspace resolver honors the env redirect without firing the v0.8 deprecation warning. The warning was written to stderr, captured via 2>&1, and compared against the command's stdout — causing the "empty queue: expected 'ran'" assert to fail on any host where SUTANDO_WORKSPACE is set. sutando-migrate.test.sh: two fixes: 1. Replace SUTANDO_WORKSPACE="$DEST" with SUTANDO_MIGRATE_DEST="$DEST" in RUN_MIGRATE(). Since v0.8, the workspace resolver ignores SUTANDO_WORKSPACE (and the --respect-env path calls sutando-config.sh which also ignores it), so the migration was running against the real workspace instead of the test fixture. SUTANDO_MIGRATE_DEST is the test hook that takes precedence unconditionally in the dest-resolution block. 2. Fix `stat -f %m` → `stat -c %Y || stat -f %m` in the mtime assertion (same GNU/BSD portability pattern as the other suites fixed in this branch). 3. Widen INITIAL_BACKUP_ID regex to match "dest empty; placeholder backup →" (printed on first commit to an empty dest) in addition to "backup →". The old regex only matched the non-empty-dest form, causing `grep` to exit 1 under set -euo pipefail and abort the test before the assertion block. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QB3qZQrwponfum2JFNvHeh
bf77ea0 to
55b5de2
Compare
sonichi#1961 more) On Ubuntu, mktemp -d creates TMP_E2E under /tmp/ with the sutando-startup- prefix, so `ls /tmp/sutando-startup-*` in test 8 matched the directory and listed its CONTENTS (which changed between PRE and POST). Change the glob to *.log — actual startup log files always have that suffix; directories never do. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QB3qZQrwponfum2JFNvHeh
55b5de2 to
a60650c
Compare
|
Split done. The sync-workspace change is now in #1997 (separate draft PR). This branch now carries only the 4 shell-test portability commits (+65/-16). PR title and description updated. The stacked agent-api commit you mentioned was already removed in an earlier rebase — GitHub shows 4 commits, all tests/. CI re-running on the new head. |
|
Automated mergeability review (2026-07-09) Signal: NOT MERGEABLE Current blocker(s):
This is a conservative backlog triage signal. After the blockers are cleared, please rerun checks and request a focused review. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Delta review of a60650c (startup-self-log-tmp T8) + the main merge. LGTM — CI green across the board.
Both prior scope notes addressed:
- The stacked #1969 agent-api commit is gone from this branch (confirmed in the current commit list — only the 4 shell-test commits + the main merge remain).
- The sync-workspace behavior change was split out to #1997, as noted in the updated PR description. This PR is now exactly what its title says.
The new T8 fix is the correct root cause, not a mask. On Ubuntu, mktemp -d puts $TMP_E2E under /tmp/ where it matches the bare sutando-startup-* glob; ls of a directory lists its contents, and test 7 writes into that directory between the PRE and POST snapshots — hence the false diff. Narrowing to /tmp/sutando-startup-*.log (tests/startup-self-log-tmp.test.sh:96,106) matches how startup.sh actually names the log (/tmp/sutando-startup-<ts>-$$.log, src/startup.sh:23), and the added | sort makes the PRE/POST comparison order-stable. The explanatory comment is good — this failure mode is non-obvious.
No issues found in the merge commit. Ready from my side (owner merges).
…ble-stat-hermetic-env # Conflicts: # tests/start-cli-claude-config-dir.test.sh # tests/startup-env-token-persist.test.sh # tests/sutando-migrate.test.sh
liususan091219
left a comment
There was a problem hiding this comment.
Codex full-review sweep: checked against the 7 triage CR classes (source-grep tests, incomplete fix, band-aid, naming, false claim, live-test policy, hardcoded paths) - none fire. Approving.
review-reason: codex-clean per full-review sweep
Approval withheld pending owner go (resequenced: CRs first). Will re-approve.
|
LGTM. I diffed the GNU/BSD Qingyun's Personal Codex Agent |
|
@cla-assistant check |
What
Fixes 8 shell test suites that were failing in Ubuntu CI (issue #1961) due to platform differences between GNU coreutils (
stat) and macOS BSD stat.Key fixes:
statordering: BSDstat -f %m fileworks on macOS; GNUstattreats-fas--file-system. Change_same_inodeand_mtimehelpers to try-cfirst (GNU) and fall back to-f(BSD rejects-ccleanly to stderr).startup-env-token-persist.test.sh: Use Python-based_perm/_mtimehelpers instead of jugglingstatsyntaxes directly — portable path for permission/mtime checks.codex-bounded.sh+migration_safety_helpers.sh: Guard the watchdog arithmetic against poisoned output whenstatsyntax doesn't match.cron-gate.shT2/T4: AddSUTANDO_TEST_MODE=1for the workspace-path check so the$SUTANDO_WORKSPACEoverride (disabled in prod since fix(workspace-v0.8): strip $SUTANDO_WORKSPACE from resolver + colored warnings [DRAFT] #1440) re-enables in test.sync-workspace.shbehavior change was split into #1997 per reviewer request.Closes #1961