Skip to content

Conversation

@jjasinsk-amd
Copy link
Collaborator

@jjasinsk-amd jjasinsk-amd commented Jan 12, 2026

Changes

This PR introduces a summarization agent and exposes its endpoint via the MCP bridge

Features

  • MCP Bridge Integration: Data is sent as multipart/form-data to leverage OCR capabilities from GAIA and Lemonade Server.
  • Streaming Responses: Summarization results are streamed back to the frontend in real time.
  • Iterative Summarization: The summary is produced iteratively in order to minimize TTFT.
  • PDF Text Caching: Extracted text from PDF files is cached for improved performance.
  • Preservation of Existing Features: Original features of the summarization app (transcripts, e-mail summarization) remain functional.

@kovtcharov-amd
Copy link
Collaborator

@claude review this PR.

@github-actions
Copy link

github-actions bot commented Jan 12, 2026

Claude finished @kovtcharov-amd's task —— View job


PR Review: Summarization Agent

  • Read PR diff and understand changes
  • Review src/gaia/agents/summarize/agent.py
  • Review src/gaia/mcp/mcp_bridge.py changes
  • Review test files
  • Review other changed files
  • Provide comprehensive feedback

Summary

This PR introduces a well-structured SummarizerAgent that consolidates summarization functionality into a reusable agent, with MCP bridge integration for streaming summarization. The overall architecture is good, but there are several issues that should be addressed before merging.


Issues to Address

Critical

  1. Copyright Header Year Inconsistency (src/gaia/agents/summarize/agent.py:1, prompts.yml:1)

    • Files use 2025-2026 but project standard is 2024-2025
    • Should match the pattern in CLAUDE.md: Copyright(C) 2024-2025 Advanced Micro Devices, Inc.
  2. Silenced Exceptions Hide Errors (src/gaia/agents/summarize/agent.py:96-101, 203-204, 336, 371-372, 375-376, 676-678)

    • Multiple bare except Exception: blocks with pass or break hide potential errors
    • Example at line 336: except Exception: just breaks silently, making debugging difficult
    • At minimum, log warnings instead of silently ignoring errors

High Priority

  1. Potential Security: Temp File Cleanup Race Condition (src/gaia/mcp/mcp_bridge.py:402-409)

    with tempfile.NamedTemporaryFile(delete=False, suffix=ext or ".pdf") as tmpfile:
        buf = file_rec.get("file_object")
        buf.seek(0)
        shutil.copyfileobj(buf, tmpfile)
        tmpfile_path = tmpfile.name
    • Temp file created with delete=False and cleaned up later - ensure cleanup happens in all error paths
    • Consider using a context manager pattern that guarantees cleanup
  2. Missing Style Validation in Streaming (src/gaia/agents/summarize/agent.py:430)

    • summarize_stream validates style but _execute_summarize in mcp_bridge accepts any style string
    • Invalid styles could cause runtime errors during streaming
  3. Hardcoded Model Default (src/gaia/agents/summarize/agent.py:109)

    DEFAULT_MODEL = "Qwen3-4B-Instruct-2507-GGUF"
    • This differs from the project default Qwen2.5-0.5B-Instruct-CPU mentioned in CLAUDE.md
    • Consider using from gaia.llm.lemonade_client import DEFAULT_MODEL_NAME for consistency

Medium Priority

  1. Token Counting is Very Approximate (src/gaia/agents/summarize/agent.py:29-37)

    def count_tokens(self, text: str) -> int:
        chars = len(text)
        words = len(text.split())
        est_by_chars = chars // 4
        est_by_words = int(words * 1.3)
        num_tokens = max(est_by_chars, est_by_words)
    • This approximation may lead to context window overflows or under-utilization
    • Consider adding a safety margin or documenting the limitation
  2. Verbose Logging During Iteration (src/gaia/agents/summarize/agent.py:319-321)

    self.log.info(
        f"[LLM Prompt - chunk {i+1}/{len(chunks)}] {base_prompt}..."
    )
    • Logs the entire prompt which can be very large - truncate in log message
  3. Repeated Code in Fallback Path (src/gaia/agents/summarize/agent.py:336-372)

    • The fallback logic in _iterative_summary_events duplicates the main streaming logic
    • Consider extracting to a helper method

Low Priority / Suggestions

  1. Missing Type Hints in Test File (tests/test_summarizer.py)

    • Test methods could benefit from return type hints for consistency
  2. Test File Date in Future (tests/test_summarizer.py:233)

    "Document generated for testing purposes - Date: 2026-01-08",
    • Hardcoded future date - consider using dynamic date or removing
  3. Redundant Path Conversion (src/gaia/agents/summarize/agent.py:838-840)

    if not isinstance(file_path, Path):
        file_path = Path(file_path)
    • This pattern appears multiple times - consider a decorator or single entry point validation

Positive Aspects

  • Clean Separation of Concerns: Agent logic cleanly separated from app wrapper
  • Comprehensive Test Coverage: Good integration tests covering CLI, streaming, and error cases
  • Streaming Support: Iterative summarization with SSE streaming is well-implemented
  • Caching: PDF text caching improves performance for repeated operations
  • Content Type Detection: Robust detection with heuristics + LLM fallback
  • Style System: Flexible YAML-based prompt configuration

Recommendation

Request Changes - The copyright header issue and silent exception handling should be addressed before merging. The other items are improvements that could be addressed in follow-up PRs.


@kovtcharov-amd kovtcharov-amd self-requested a review January 12, 2026 21:42
@github-actions github-actions bot added dependencies Dependency updates devops DevOps/infrastructure changes agents Agent system changes mcp MCP integration changes cli CLI changes tests Test changes labels Jan 13, 2026
@kovtcharov-amd
Copy link
Collaborator

Is this a good replacement for the existing summarizer that's already checked in?

@jjasinsk-amd
Copy link
Collaborator Author

Is this a good replacement for the existing summarizer that's already checked in?

The core features of the original summarizer application such as processing transcripts and emails were not changed. This PR adds the possibility to summarize PDF files and encapsulates the summarization logic within a dedicated agent, improving modularity and reusability.

@jjasinsk-amd jjasinsk-amd added this pull request to the merge queue Jan 16, 2026
Merged via the queue into main with commit baf2fe1 Jan 16, 2026
68 checks passed
@jjasinsk-amd jjasinsk-amd deleted the jjasinsk/summarize_agent branch January 16, 2026 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents Agent system changes cli CLI changes dependencies Dependency updates devops DevOps/infrastructure changes mcp MCP integration changes tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants