Skip to content

fix: bundle small bug fixes originally submitted by @CodeLine9#813

Open
kovtcharov wants to merge 8 commits intomainfrom
fix/codeline9-bundle
Open

fix: bundle small bug fixes originally submitted by @CodeLine9#813
kovtcharov wants to merge 8 commits intomainfrom
fix/codeline9-bundle

Conversation

@kovtcharov
Copy link
Copy Markdown
Collaborator

Summary

Bundles 7 small, disjoint bug fixes originally submitted by @CodeLine9 in #804#810. He closed them himself citing bandwidth; the underlying issues (#642, #787, #788, #789, #791, #792, #793) are all still open and the fixes are still valid. Each commit preserves CodeLine9's authorship.

All fixes target different files, so the bundle carries zero merge conflicts and is easy to review as a single unit.

Fixes (one commit each)

Attribution

All 7 commits carry CodeLine9 ty0907@hotmail.com as Author:. cc @CodeLine9 — thank you for the triage and the clean small-diff style; these are all worth having in main.

Test plan

  • python util/lint.py --black passes on touched Python files (pre-existing mcp_bridge.py failure from fix(mcp): guard JSON-RPC handler against non-dict body #803 is unrelated)
  • Launch Lemonade from gaia on Linux; confirm --ctx-size is respected (previously silently dropped)
  • gaia chat --ui, upload a file, delete it from disk, wait >30s; confirm /api/documents flips to indexing_status: "missing"
  • Agent UI webui builds (cd src/gaia/apps/webui && npm run build)
  • Mobile viewport: table in chat message scrolls horizontally
  • Smoke: /api/system/download-model survives a forced gc.collect() cycle mid-download

@github-actions github-actions bot added llm LLM backend changes electron Electron app changes performance Performance-critical changes labels Apr 20, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Summary

This is a well-curated bundle of 7 small, disjoint bug fixes in different files — trivial to review as a unit with zero merge risk. The fixes are each minimal, targeted, and preserve the original contributor's authorship. The highest-impact item is the shell=True removal in lemonade_client.py, which was silently dropping --ctx-size and --log-level for all Linux/macOS users. The rest are genuine reliability / correctness improvements.

Issues Found

🟢 Minor — Inconsistent extension-parsing pattern in same file (src/gaia/apps/webui/src/components/FileBrowser.tsx:279)

The fix updates two call sites (lines 284, 341) to use the safer lastIndexOf('.') approach, but the pre-check on line 279 still uses the older f.includes('.') ? '.' + f.split('.').pop()... style. Both happen to be correct here (the includes('.') guard covers the dotless case), but the inconsistency is a trap for future edits. Consider one helper:

        const hasSupported = files.some(f => {
            const dotIdx = f.lastIndexOf('.');
            const ext = dotIdx > 0 ? f.slice(dotIdx).toLowerCase() : '';
            return !ext || isExtensionSupported(ext);
        });

Not blocking — feel free to defer as it's cosmetic.

🟢 Minor — Partial lock coverage in SSE handler (src/gaia/ui/sse_handler.py:698,714,729,734)

The new self._confirm_lock protects the reset/set of _confirm_event and _confirm_result, but writes to self._confirm_id (lines 698, 714, 729, 734) remain outside the lock. In practice this is fine — confirm_id isn't consulted by resolve_tool_confirmation, and Python attribute writes are atomic under the GIL — so it doesn't break correctness. Worth a short comment so the next maintainer doesn't add a read-after-write race:

        # NOTE: _confirm_id is written outside the lock on purpose — it is only
        # read by confirm_tool_execution itself, never by resolve_tool_confirmation.
        self._confirm_id = confirm_id

🟢 Minor — kill_process_on_port still uses shell=True with interpolated pids (src/gaia/util.py:42,67)

The int(port) cast closes the externally-caller shell-injection vector on port, which is the stated goal. But the same function later does subprocess.run(f"taskkill /F /PID {pid}", shell=True, ...) and kill -9 {pid} where pid came from parsing netstat/lsof stdout. In a compromised environment these are trusted outputs, so severity is low — and the PR description already flags "consolidate onto the psutil-based impl already used elsewhere" as a follow-up. Just confirming that follow-up is the right call; no change requested in this PR.

Strengths

  • lemonade_client.py fix is high-value and minimally invasive. subprocess.Popen(list, shell=True) on POSIX silently collapses to sh -c base_cmd[0], dropping every subsequent arg — so --ctx-size 32768 was being silently ignored on Linux/macOS. Two-line fix, correct, no test churn.
  • document_monitor.py fix is defensively scoped. The self._db.update_document_status(doc_id, "missing") is correctly gated inside if status != "missing":, so it both flips the DB once and stops the log spam on subsequent poll cycles — no extra state plumbing needed.
  • asyncio.create_task GC fix is the canonical idiom. _background_tasks.add(task) + task.add_done_callback(_background_tasks.discard) is exactly the pattern recommended in the CPython docs and matches PEP 3156. Critical for the 7200s download timeout.

Verdict

Approve. All 7 fixes are correct, minimally scoped, and individually defensible. The minor items above are style/comment nits, not blockers. Nice bundling of orphaned-but-valid external contributions — preserving @CodeLine9 as Author: on each commit is the right call.

Fixes `python util/lint.py --black` failure on main. Pure formatting — no
behaviour change. Line width regression was introduced in #803 where the
error dict exceeded black's 88-char limit.
@github-actions github-actions bot added the mcp MCP integration changes label Apr 20, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Summary

Clean bundle of seven disjoint, small-diff bug fixes preserving @CodeLine9's authorship. Each fix targets a different file so there's zero cross-cutting risk, and the two highest-impact items (Lemonade shell=True regression and port-injection hardening in util.py) are both sound. Overall quality is high — approve after considering a couple of minor follow-ups below.

Issues Found

🟢 Minor — resolve_tool_confirmation lock only partially covers the race (src/gaia/ui/sse_handler.py:710-736)

The new _confirm_lock protects the reset (confirm_tool_execution L695–697) and set (resolve_tool_confirmation L744–750) critical sections, which closes the specific "early-approval gets clobbered" race called out in #792. However, the subsequent reads in confirm_tool_execution are still unlocked:

  • self._confirm_event.wait(...) at L717 and the final result = self._confirm_result at L733 are read outside the lock.
  • _confirm_id is never re-validated inside resolve_tool_confirmation, so a stale HTTP confirm from a previous request can still satisfy the next confirm_tool_execution call if it arrives in the window between event-creation and event-wait.

For this PR I'd still merge — the fix strictly improves correctness over the prior state — but worth a follow-up issue to either (a) use a queue.Queue keyed by confirm_id or (b) validate confirm_id inside resolve_tool_confirmation before calling .set().

🟢 Minor — update_document_status docstring missing "missing" status (src/gaia/ui/database.py:691)

document_monitor.py now writes "missing" through update_document_status(doc_id, "missing"), and the monitor already treats "missing" as a first-class state at document_monitor.py:152. The docstring for update_document_status still lists only ('pending', 'indexing', 'complete', 'failed', 'cancelled'):

            status: New status ('pending', 'indexing', 'complete', 'failed', 'cancelled', 'missing').

Not blocking, and unrelated to the core fix, but a one-line drive-by is cheap and keeps the docstring honest.

🟢 Minor — extension-parsing pattern still inconsistent in neighbouring files

The two call sites in FileBrowser.tsx are fixed correctly (lastIndexOf('.') + > 0 guard handles README, .bashrc, archive.tar.gz, README.md all correctly). The same pattern survives in:

  • src/gaia/apps/webui/src/components/DocumentLibrary.tsx:216
  • src/gaia/apps/webui/src/components/UnsupportedFeature.tsx:239
  • src/gaia/apps/webui/src/components/FileBrowser.tsx:106, :279, :528

These all use the includes('.') short-circuit, so they don't produce .readme for dotless names — but they do produce .bashrc for dotfiles, which is a subtly different miscategorisation. Not in scope for this PR; worth a follow-up to extract a shared getExtension(filename) helper and apply the lastIndexOf('.') > 0 rule consistently.

Strengths

  • Lemonade shell=True removal is the right call (src/gaia/llm/lemonade_client.py:706,719) — Popen(list, shell=True) on POSIX invokes sh -c "<first element>" and silently drops serve/--log-level/--ctx-size. Linux/macOS users were effectively running the server with none of the configured flags. High-impact, low-risk fix.
  • Port-injection fix casts before interpolation (src/gaia/util.py:12-15) — hardening an f-string into shell=True lsof/taskkill commands by forcing int(port) at the top is the correct minimal fix. The follow-up candidate called out in the PR description (consolidate onto the existing psutil impl) is the right long-term direction, but this is the right short-term patch.
  • _background_tasks set with add_done_callback(_background_tasks.discard) is the idiomatic Python pattern (src/gaia/ui/routers/system.py:32-33,461-466,490-495) — matches CPython docs exactly. Particularly important for the 7200s download timeout path where GC pressure mid-download was a real reliability risk.
  • Authorship preserved — CodeLine9 retained as Author: on each commit; good contributor etiquette.

Verdict

Approve. No blocking issues. The three 🟢 Minor items above are all follow-up candidates that don't need to hold up the bundle. Seven focused fixes landing together is materially better than seven stalled PRs sitting in the closed queue.

@kovtcharov kovtcharov enabled auto-merge April 20, 2026 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

electron Electron app changes llm LLM backend changes mcp MCP integration changes performance Performance-critical changes

Projects

None yet

2 participants