fix(ci): remove || true from test workflows, add timeouts and log uploads

[kovtcharov-amd]

Five CI workflows were silently swallowing test failures via || true, so broken tests never actually failed the build. Now pytest exits propagate directly — if a test fails, CI fails. The CLI integration job also checks all three test-category exit codes before completing instead of ignoring them. Jobs that lacked timeout-minutes (lint, unit tests) now have one, and lemonade.log is uploaded as a build artifact on failure instead of being deleted.

Test plan

• test_code_agent.yml — confirm the three pytest steps fail the job when tests fail (no more || true)
• test_gaia_cli_linux.yml — confirm the step exits non-zero when any of summarizer/RAG/lemonade tests fail; confirm lemonade.log appears in artifacts on failure
• test_mcp.yml (Linux) — confirm gaia mcp test failure fails the step; confirm gaia mcp stop || true is still present in cleanup
• lint.yml — confirm timeout-minutes: 15 is present on the lint job
• test_unit.yml — confirm timeout-minutes: 30 is present on the unit-tests job
• Verify no changes to claude.yml, publish.yml, or npm audit continue-on-error steps

Closes #876

[github-actions]

Clean CI hygiene PR that correctly applies the "fail loudly" principle. Five workflows were silently swallowing test failures via || true; now pytest exits propagate directly. No blocking issues.

Summary

This is exactly the right fix: || true on a test step is a lie — it tells the runner everything is fine when it isn't, and regressions hide for weeks. The exit code aggregation in the Linux CLI test and the lemonade.log artifact upload are practical improvements on top of the core behavioral change.

One observation worth tracking after merge: handle_mcp_test() in src/gaia/cli.py:6331 never calls sys.exit(1) — every failure path prints ❌ ... and returns without raising. So gaia mcp test exits 0 regardless of whether the actual MCP round-trip succeeds. That made the removed || true redundant (not load-bearing), so the Linux MCP step won't break CI — but it also means the step won't catch a real gaia mcp test regression either. Pre-existing issue, not introduced here, but worth a follow-up.

Issues Found

🟢 Minor — Stale comment contradicts the new behavior (test_mcp.yml:199)

After the || true is removed, the inline comment on line 199 still reads # Run MCP tests using gaia CLI (expected to fail without Lemonade). The PR intent is the opposite: failures should now propagate. As noted above, the step is harmless in practice (the command exits 0 either way), but a future reader will be confused about whether gaia mcp test is supposed to be blocking.

          # Test MCP bridge connectivity and tool dispatch

🟢 Minor — Missing newline at EOF (test_code_agent.yml:145)

The file still ends without a trailing newline (pre-existing, shown as \ No newline at end of file in the diff). Low priority but trivially fixable since the file is being touched.

Strengths

• Correct application of the "fail loudly" rule. Removing || true from all three pytest steps in test_code_agent.yml and from gaia mcp test in test_mcp.yml is the right move. The previous "Note: Tests are currently non-blocking" language was a red flag that these tests had no enforcement teeth.
• Exit code aggregation in the CLI test is well-written. The ${TEST_EXIT:-0} pattern with FIRST_FAILURE accumulation (test_gaia_cli_linux.yml:97-105) correctly surfaces the first failing category's exit code without masking the others.
• Log preservation via artifact upload. Replacing rm -f lemonade.log with an upload-artifact@v7 step (with if-no-files-found: ignore) gives developers something concrete to debug against on failure without risking CI failure if the log doesn't exist.
• Timeout values are consistent with repo conventions. timeout-minutes: 15 for lint and timeout-minutes: 30 for unit tests match the values used in test_examples.yml, test_agent_mcp_server.yml, and test_api.yml.

Verdict

Approve. No blocking issues. The stale comment is the only cleanup remaining and it's a nit. The gaia mcp test exit-code gap is worth a separate issue or follow-on PR.

[github-actions]

Solid CI hardening — removing the || true scaffolding from test_code_agent.yml makes those three jobs genuinely blocking for the first time, and adding timeouts + log artifact upload are good ops hygiene. One stale comment creates a misleading signal in the MCP workflow; everything else is either clean or intentional.

---

Issues Found

🟡 Stale comment contradicts the intent in test_mcp.yml:199

The || true was removed from gaia mcp test, but the comment immediately above it still reads:

# Run MCP tests using gaia CLI (expected to fail without Lemonade)

This is contradictory: if the step is "expected to fail," the || true should still be there; if it's not expected to fail anymore, the comment must go. Reading handle_mcp_test in src/gaia/cli.py:6331-6410, the function always exits 0 — every error path either returns or prints and falls through without calling sys.exit(1). So the || true was already redundant, and this removal is safe — but the stale comment will confuse the next person who touches this file.

          # Test MCP bridge connectivity and tool dispatch
          echo "Testing MCP bridge functionality..."
          gaia mcp test

🟢 Weak system-prompt assertions in tests/test_code_agent.py:73-74

The updated assertions are much harder to regress on than the old "expert Python developer" / "Python" checks:

self.assertIn("code assistant", prompt)
self.assertIn("tool", prompt.lower())

"tool" will appear in nearly any agent system prompt. A substring that is more specific to CodeAgent's actual prompt (e.g. the tool-call format sentinel or a phrase from FileIOAgent's injected instructions) would give this test real signal. Worth revisiting when the prompt stabilises.

🟢 Missing newline at end of test_code_agent.yml

The \ No newline at end of file marker on the final echo "===..." line was already present before this PR, but this diff touched the surrounding lines and is a good opportunity to close it.

          echo "================================================================"

(Add a trailing newline after the final echo in your editor — the diff marker will disappear.)

---

Strengths

• Dead-code removal in test_code_agent.yml is clean. The UNIT_TEST_EXIT / INTEGRATION_TEST_EXIT / WORKFLOW_TEST_EXIT capture-and-report pattern was entirely vestigial after || true made every exit code 0 — cutting it reduces the workflow by ~30 lines with no loss.
• allowed_paths=[self.test_dir] in setUp correctly scopes CodeAgent file operations to the temp directory. The previous constructor call would have allowed the agent to read/write anywhere, which was a latent risk in the test suite.
• Artifact upload for lemonade.log is the right call — it's the most useful log for diagnosing integration failures and was being deleted before reviewers could see it.
• CLI integration ::warning:: approach is a reasonable compromise given that only Qwen3-0.6B-GGUF is pulled in CI and summarizer/RAG tests legitimately need the full model. Surfacing the failure as a GHA annotation is louder than the old silent || true without blocking unrelated jobs.

---

Verdict

Approve with suggestions. The stale MCP comment is the only thing that should be fixed before merge — one-line change. Everything else either lands correctly or is a pre-existing issue outside this PR's scope.

[github-actions]

Solid CI hygiene fix — removing the || true blanket in test_code_agent.yml means broken code tests now actually block merges, and the updated setUp fixtures + adjusted system-prompt assertions bring the test file in sync with the current implementation. Two minor issues worth addressing before merge.

---

Issues

🟢 Stale comment contradicts the intent of the change (test_mcp.yml:199)

The || true was removed from gaia mcp test, but the comment directly above it was not updated:

# Run MCP tests using gaia CLI (expected to fail without Lemonade)
echo "Testing MCP bridge functionality..."
gaia mcp test

This comment now misleads readers into thinking the step is still expected to fail — and may prompt someone to re-add || true in the future. Worth noting that handle_mcp_test() in src/gaia/cli.py:6331 catches all exceptions internally and always exits 0 (connection failures return early on line 6355; tool errors are printed but not re-raised). The || true removal is therefore a no-op in practice, and the test-plan item "confirm gaia mcp test failure fails the step" won't actually hold true. Update or drop the comment:

          echo "Testing MCP bridge functionality..."
          gaia mcp test

🟢 Missing newline at EOF (test_code_agent.yml)

The diff ends with \ No newline at end of file. POSIX and the project's Black/isort toolchain both expect files to end with \n. Adding one prevents unnecessary diff noise in future PRs.

---

Strengths

• Three pytest phases in test_code_agent.yml now actually block CI. The before-state (store exit code, echo warning, continue) gave the illusion of enforcement — now a failing unit, integration, or workflow test actually fails the job. This is the most impactful change in the PR.
• allowed_paths=[self.test_dir] in both setUp methods correctly matches the CodeAgent's path-allowlist requirement; and updating the system-prompt assertions to "code assistant" / "tool" makes them track the real prompt in base_prompt.py:37 instead of stale strings.
• lemonade.log artifact upload (if: always()) and the ::warning:: pattern for the CLI integration tests are both good additions — the first makes post-failure debugging tractable, the second surfaces model-availability failures without blocking CI on an infrastructure constraint that can't be fixed at the PR level.

---

Verdict

Approve with suggestions. The stale comment in test_mcp.yml and the missing EOF newline are both trivial. The core change — making code-agent test failures blocking — is correct and well-tested.

[github-actions]

Clean, well-scoped CI hardening with the right design decisions — continue-on-error: true replacing || true on the workflow step, direct pytest propagation on unit/integration tests, and proper log artifact upload. One area needs clarification before merge.

---

Issues Found

🟡 Stale comment contradicts the || true removal (test_mcp.yml:199)

The # Run MCP tests using gaia CLI (expected to fail without Lemonade) comment at line 199 was not touched, but || true was removed from the line immediately below it. As written, the comment says failure is expected without Lemonade but the step will now exit non-zero and fail the Linux MCP job if that's true. In bash's -e mode (GHA default), that kills the step.

Two possible situations — only one of them is right, and the code as merged implies neither:

• If gaia mcp test now passes without Lemonade → delete or rewrite the comment so future readers don't have to puzzle over the contradiction.
• If gaia mcp test still exits non-zero without Lemonade → the Linux MCP job is now broken; reinstate || true or move this step behind a continue-on-error: true (matching the pattern used on the workflow tests in test_code_agent.yml).

The Windows job has the same comment at line 117 (# Don't fail on this test) plus line 118's Write-Host, both now equally stale — clean those up too.

          # `gaia mcp test` probes the MCP bridge; no Lemonade required.
          echo "Testing MCP bridge functionality..."
          gaia mcp test

(Update for the Linux job at line 199–201; mirror for the Windows job at lines 114–118.)

---

🟢 Missing trailing newline at EOF (test_code_agent.yml)

The diff shows \ No newline at end of file. POSIX requires text files to end with a newline; some tooling (diff, cat, grep) behaves oddly without it.

          echo "================================================================"

(Add a blank line or just a newline after the last echo at the end of the file.)

---

Strengths

• continue-on-error: true on the workflow step is exactly the GAIA-approved pattern (per CLAUDE.md) — tolerates a known-unresolvable failure at the step level without silently degrading the tests that can pass. The inline comment explaining WHY it's there is exactly right.
• allowed_paths=[self.test_dir] in TestCodeAgent.setUp and TestCodeAgentIntegration.setUp is a meaningful correctness fix — CodeAgent was previously constructed without sandbox scope in tests.
• Removing the UNIT_TEST_EXIT / INTEGRATION_TEST_EXIT dance simplifies the workflow considerably; the exit code now propagates from pytest directly rather than through fragile shell arithmetic.
• lemonade.log upload with if-no-files-found: ignore is the right pattern — artifact is available when debugging is needed, silently skipped when the log wasn't produced.
• Timeout additions (lint: 15m, unit-tests: 30m) prevent silent indefinite hangs in the two jobs that were missing them.

---

Verdict

Approve with suggestions. The stale comment in test_mcp.yml is the only thing worth resolving before merge — it either hides a CI break or misleads future readers. The EOF newline is a nit. Everything else is solid.

[itomek]

This is the right fix and it correctly distinguishes "remove the lie" (|| true on pytest steps) from "tolerate a known-unfixable-in-CI failure" (step-level continue-on-error on the LLM-dependent workflow tests, which is the allowed pattern). Verified the new exit-code aggregation in test_gaia_cli_linux.yml: TEST_EXIT / RAG_TEST_EXIT / LEMONADE_TEST_EXIT are set via || VAR=$? in the same shell as the aggregation loop, so it correctly emits a ::warning:: rather than always seeing 0. The genuinely fail-loud teeth are the code-agent unit/integration pytest steps — the real win. One inline note on the stale test_mcp.yml comments. Separately (couldn't inline it since cli.py isn't in this diff): handle_mcp_test in src/gaia/cli.py never exits non-zero on any path, so removing || true on the gaia mcp test step is inert and the test-plan item "confirm gaia mcp test failure fails the step" can't pass as written — worth a follow-up issue to give that real teeth. Approving.