Skip to content

Fix harness setups#1441

Merged
xeophon merged 1 commit into
fix/pi-earendil-packagefrom
fix/pi-harness-setup
May 22, 2026
Merged

Fix harness setups#1441
xeophon merged 1 commit into
fix/pi-earendil-packagefrom
fix/pi-harness-setup

Conversation

@xeophon
Copy link
Copy Markdown
Member

@xeophon xeophon commented May 22, 2026

Summary

  • stack this targeted shell fix on Use Earendil Pi package #1442 so the Pi package change stays separate
  • replace bash-only double-bracket workdir checks with POSIX single-bracket checks in OpenCode, Pi, and Terminus 2 generated harness scripts
  • add focused assertions that generated scripts use the POSIX checks in the affected paths

Verification

  • uv run ruff format verifiers/v1/packages/harnesses/opencode.py verifiers/v1/packages/harnesses/pi.py verifiers/v1/packages/harnesses/terminus_2.py tests/test_v1_harbor_cli.py
  • uv run ruff check --fix verifiers/v1/packages/harnesses/opencode.py verifiers/v1/packages/harnesses/pi.py verifiers/v1/packages/harnesses/terminus_2.py tests/test_v1_harbor_cli.py
  • uv run pytest tests/test_v1_harbor_cli.py -q
  • generated OpenCode and Pi MCP setup scripts run successfully under /bin/dash

Note

Fix harness run scripts to use POSIX [ -z ] instead of bash [[ -z ]]

Replaces [[ -z "$*_WORKDIR" ]] with [ -z "$*_WORKDIR" ] in the generated shell scripts for the Pi, OpenCode, and Terminus 2 harnesses. The double-bracket form is a bash extension that fails in POSIX sh environments used by these harnesses.

Macroscope summarized 0234bde.

macroscopeapp[bot]
macroscopeapp Bot previously approved these changes May 22, 2026
@macroscopeapp
Copy link
Copy Markdown

macroscopeapp Bot commented May 22, 2026

Approvability

Verdict: Approved

This PR makes a mechanical shell syntax change from bash-specific [[ ]] to POSIX-compatible [ ] for empty string checks, improving portability. The test additions verify this syntax. The review comments appear to address pre-existing code behavior, not changes in this PR.

You can customize Macroscope's approvability policy. Learn more.

@xeophon xeophon force-pushed the fix/pi-harness-setup branch from fdb2c5e to f27a6bb Compare May 22, 2026 19:14
@xeophon xeophon changed the base branch from main to fix/pi-earendil-package May 22, 2026 19:15
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f27a6bb588

ℹ️ 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".

Comment thread verifiers/v1/packages/harnesses/pi.py Outdated
apt-get -o Acquire::Retries=3 update -qq
apt-get -o Acquire::Retries=3 install -y -qq curl ca-certificates gnupg > /dev/null 2>&1
mkdir -p /etc/apt/keyrings
curl -fsSL https://deb.nodesource.com/gpgkey/nodesource-repo.gpg.key | gpg --dearmor | tee /etc/apt/keyrings/nodesource.gpg > /dev/null
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Preserve pipeline failures when importing NodeSource key

The new install script pipes curl | gpg --dearmor | tee ... under set -e, but without pipefail only tee’s exit code is checked. If curl or gpg fails (for example a transient download/trust failure), the script can continue and then apt-get update may still return success with warnings, causing apt-get install nodejs to fall back to distro Node rather than Node 22. This silently defeats the main purpose of the change and can reintroduce Pi runtime mismatches.

Useful? React with 👍 / 👎.

@xeophon xeophon force-pushed the fix/pi-harness-setup branch from f27a6bb to 05f0fce Compare May 22, 2026 21:51
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 05f0fce3aa

ℹ️ 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".

Comment thread verifiers/v1/packages/harnesses/pi.py Outdated
Comment on lines +120 to +124
cat > "$HOME/.pi/agent/settings.json" <<'EOFSETTINGS'
{
"packages": [
"npm:pi-mcp-adapter"
]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve existing Pi settings when enabling MCP adapter

The new setup script unconditionally rewrites ~/.pi/agent/settings.json with only a packages array, which discards any pre-existing global Pi settings (for example npmCommand, model defaults, retry/session settings, or other required packages). This regression appears when the sandbox image or a reused home directory already contains Pi configuration, and can break runs that depend on those settings; the prior pi install ... -l path did not replace the whole global settings file.

Useful? React with 👍 / 👎.

@xeophon xeophon force-pushed the fix/pi-harness-setup branch from 05f0fce to 8760670 Compare May 22, 2026 22:01
@xeophon xeophon force-pushed the fix/pi-harness-setup branch from 8760670 to 0234bde Compare May 22, 2026 22:07
@xeophon xeophon changed the title Fix Pi harness setup Fix harness setups May 22, 2026
@xeophon xeophon merged commit 52cc392 into fix/pi-earendil-package May 22, 2026
6 checks passed
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.

1 participant