Skip to content

fix: apply reviewed maintenance repairs for #3336 - #3691

Merged
kovtcharov-amd merged 1 commit into
amd:fix/installer-stdio-agentfrom
kovtcharov:codex/maintenance-pr-3336
Sep 18, 2026
Merged

kovtcharov-amd merged 1 commit into
amd:fix/installer-stdio-agentfrom
kovtcharov:codex/maintenance-pr-3336

Conversation

@kovtcharov

Copy link
Copy Markdown
Contributor

Follow-up fixes for #3336. Review nits addressed locally: smoke-test backend allowlist now points maintainers at the stdio startup-ping contract; wrong-transport installer guards no longer depend on double-quote style.

This PR targets the original feature branch because the current account cannot push to amd/gaia. It carries the prepared maintenance changes for that PR.

Test plan

  • Previously completed local validation: Validation: tests/unit/test_fetch_sidecar.py tests/unit/installer/test_install_scripts_terminal_hub.py hub/agents/gaia/python/tests/test_gen_binaries_lock.py — 76 passed, 16 existing skips (POSIX-only bootstrap tests / unavailable optional artifact environment). Source changes limited to a comment and quote-independent guards.
  • Fresh CI on the combined feature branch.
  • Remaining live-model, hardware or platform checks described in the original PR, where applicable.
  • Human review before merging the original PR.

Review nits addressed locally: smoke-test backend allowlist now points maintainers at the stdio startup-ping contract; wrong-transport installer guards no longer depend on double-quote style.
@github-actions github-actions Bot added tests Test changes sidecar Agent sidecar contract / harness labels Sep 11, 2026
@kovtcharov
kovtcharov marked this pull request as ready for review September 17, 2026 20:04
@github-actions

Copy link
Copy Markdown
Contributor

Approve

Two small follow-ups to the stdio-agent installer work: a tightened test guard and a one-line comment. The guard change is a real improvement — the old check only caught the wrong binary name when it appeared wrapped in quotes, so an unquoted use of the REST sidecar name would have slipped past it silently. I confirmed both install scripts still pass the stricter check.

Real-world evidence

N/A — this PR changes only a unit-test assertion and a code comment; no user-visible surface is touched, so there is nothing for the evidence lane to exercise. The verdict rests on static review, and I was unable to run the test suite here (every shell command in this environment failed with a sandbox error, unrelated to the PR).

🔍 Technical details

Verified

  • tests/unit/installer/test_install_scripts_terminal_hub.py:668 — dropping the embedded double quotes from the needles makes the assertion strictly stronger, and it still passes: neither gaia-agent-${platform} nor gaia-agent-$lockPlatform.exe appears anywhere in installer/scripts/install.{sh,ps1}. The real names are gaia-agent-stdio-${platform} (install.sh:476) and gaia-agent-stdio-$lockPlatform.exe (install.ps1:349), and the prose that mentions the sidecar form sits on full-line comments, which the filter above strips.
  • hub/agents/gaia/python/packaging/smoke_test.py:338 — the comment is accurate; gaia_agent/stdio.py:339 emits exactly "claude" if is_claude else "lemonade".

🟢 Minor — the guard is coupled to the shell variable names

Both needles hardcode ${platform} / $lockPlatform. If someone renames the loop variable in either script while reintroducing the sidecar artifact, this assertion goes quiet rather than failing. The functional test below it (test_the_installed_agent_is_the_stdio_artifact) is the real backstop, so this is defense-in-depth only — but a name-agnostic check would keep its value across a rename:

    for wrong in ("gaia-agent-${platform}", "gaia-agent-$lockPlatform.exe"):
        assert wrong not in code, f"install.{script} asks for the REST sidecar"
    # Name-agnostic backstop: any `gaia-agent-` followed by something other than
    # `stdio-` is the sidecar, whatever the platform variable is called.
    assert not re.search(r"gaia-agent-(?!stdio-)\$", code), (
        f"install.{script} constructs a non-stdio agent artifact name"
    )

(re is already imported at the top of the module.)

Strengths

  • The quote-stripping fix targets exactly the failure mode the surrounding comment block warns about — a green install gate followed by a broken chat — rather than just adding coverage for its own sake.
  • Pairing the static guard with a functional test that asserts on the downloaded bytes is the right shape: the static check explains the intent, the functional one proves it.

@kovtcharov-amd
kovtcharov-amd merged commit 2496b3c into amd:fix/installer-stdio-agent Sep 18, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sidecar Agent sidecar contract / harness tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants