Skip to content

test(installer): lock repo-root resolution in bash launcher scripts#1395

Open
dislovelhl wants to merge 4 commits into
amd:mainfrom
dislovelhl:fix/installer-bash-root-regression
Open

test(installer): lock repo-root resolution in bash launcher scripts#1395
dislovelhl wants to merge 4 commits into
amd:mainfrom
dislovelhl:fix/installer-bash-root-regression

Conversation

@dislovelhl
Copy link
Copy Markdown
Contributor

Why this matters

#1332 fixed the off-by-one repo-root bug in start-agent-ui.sh and build-ui-installer.sh (the bash counterparts to the PowerShell fix in #1326), but shipped with only a manual bash -n test plan — nothing in CI stops the $SCRIPT_DIR/..installer/ regression from creeping back in if either script is moved or edited again. #1328 is still open precisely because the fix was never pinned.

This adds the regression coverage: a pure-stdlib test that parses both scripts and asserts the two-levels-up traversal resolves to the real repo root, that $WEBUI_DIR/$ELECTRON_DIR point at src/gaia/apps/webui and src/gaia/electron (which exist on a fresh checkout), and that the old one-level traversal would have landed on installer/. No production code changes — the fix is already on main.

Closes #1328.

Test plan

  • python -m pytest tests/unit/installer/test_bash_script_paths.py — 3 passed against the current (fixed) scripts.
  • Sanity: temporarily reverting either script to $SCRIPT_DIR/.. makes the corresponding test fail (the guard actually guards).
  • black --check and isort --check-only clean on the new file.

Out of scope

The installer bash scripts regressed once by traversing only one level
($SCRIPT_DIR/..) from installer/scripts, landing on installer/ instead of
the repo root and breaking webui/electron path resolution. These tests pin
the two-level traversal and the WEBUI_DIR/ELECTRON_DIR assignments so the
regression can't silently return.
@github-actions github-actions Bot added the tests Test changes label Jun 4, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 4, 2026

Review

Approve. This is a tight, well-scoped regression test that pins the $SCRIPT_DIR/../.. two-level traversal in both installer bash launchers, closing the CI gap #1332 left open. I ran all three tests against the live scripts (installer/scripts/start-agent-ui.sh:50, build-ui-installer.sh:15) and confirmed they pass, and verified the guard actually fails on the old one-level traversal — reverting either script to $SCRIPT_DIR/.. makes project_root == REPO_ROOT fail, exactly as the test plan claims.

No production code changes, no security surface, no new dependencies (pure stdlib re + pathlib). No prompt-injection content in the diff.

Issues

🟢 Minor — the first test is documentation, not a guard (test_bash_script_paths.py:45)
test_old_one_level_bash_traversal_would_resolve_to_installer_dir never reads either script — it just asserts installer/scripts/.. == installer/, which is always true by directory structure. It nicely documents the regression, but the real guarding is done by the two path-resolution tests (which read the actual ../.. traversal out of the scripts and assert it lands on the repo root). Worth a one-line docstring note that this case is illustrative, or fold it into the other two — not blocking either way.

🟢 Minor — regex parsing is tight to script formatting (test_bash_script_paths.py:25,35)
The ^VAR="$(cd "$SCRIPT_DIR/..." && pwd)"$ patterns assume exact quoting/spacing, so a benign reformat of the scripts (e.g. a trailing comment or alternate path expression) would trip the assert match, "... does not assign ..." branch rather than the value assertion. That's an acceptable trade-off for a pinning test, and the assert messages are clear about what failed — just be aware future script edits may need the regex touched alongside them.

Strengths

  • Pure-stdlib, deterministic, runs anywhere a checkout exists — no Lemonade/network/fixtures needed; correctly placed under tests/unit/installer/ next to the existing installer tests.
  • Asserts against the real checkout (WEBUI_DIR.is_dir(), ELECTRON_DIR.is_dir() at src/gaia/apps/webui and src/gaia/electron), so it catches both a traversal regression and a directory move.
  • Helper functions (_script_assignment / _literal_assignment / _resolved_from_scripts_dir) keep each test readable and the failure messages actionable.
  • PR description is exemplary: leads with the why (the [Bug]: bash installer scripts (start-agent-ui.sh, build-ui-installer.sh) have the same off-by-one path bug as #1325 #1328 gap), states what's out of scope, and the test plan is verifiable.

Verdict

Approve. Both notes are 🟢 minor and optional — nothing blocks merge.

martin added 2 commits June 3, 2026 22:31
The Code Quality gate was already red on main: pylint flagged W0613
(unused-argument) in the Outlook/Gmail Protocol-parity stubs and the email
MCP send fake, plus W0102 (dangerous mutable default) on
categorization_export. None are bugs — the unused kwargs are kept for
signature parity with the Google backends (as the existing comments note),
and the export default is read-only. Suppress the parity stubs with a
targeted disable and switch the mutable default to a None sentinel so the
gate passes without weakening it globally.
@github-actions github-actions Bot added mcp MCP integration changes eval Evaluation framework changes performance Performance-critical changes agents labels Jun 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents eval Evaluation framework changes mcp MCP integration changes performance Performance-critical changes tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: bash installer scripts (start-agent-ui.sh, build-ui-installer.sh) have the same off-by-one path bug as #1325

1 participant