Skip to content

Conversation

@jinhyun95
Copy link

@jinhyun95 jinhyun95 commented Jul 22, 2025

This PR separate dialog history of multiple agents.

@JoshuaC215
Copy link
Owner

I'm supportive of separating history by agent in a better way than provided today. But I'm not immediately a fan of the other changes. If you want to open an issue for removing default agent and forcing the user to always specify an agent, we could see what others in the community think and get some discussion. Probably should be separate PRs.

@jinhyun95 jinhyun95 force-pushed the refactor/remove_hardcoded_default branch from b8558bc to 08d79dc Compare July 24, 2025 04:50
@jinhyun95 jinhyun95 changed the title refactor: Remove hard-coded default agent and unused endpoints feature: Separate dialog history of each agent Jul 24, 2025
@jinhyun95
Copy link
Author

Thanks for the review :)
I reverted the changes regarding default agent and APIs and amended the commit (08d79dc)
thread_id is now handled in service, specifically _handle_input.
PR description is edited accordingly.

@codecov
Copy link

codecov bot commented Jul 29, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.51%. Comparing base (2e6c622) to head (08d79dc).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Coverage Δ
src/service/service.py 79.50% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JoshuaC215
Copy link
Owner

So I think the simplified change creates a few problems, particularly in the interaction with the streamlit app (maybe we need to add tests for those)

  1. The /history endpoint just looks naively for the thread_id, so I think it will always return nothing since it doesn't match the format threads are being saved in. To resolve this, we might use the same pattern used by /invoke and /stream where you can optionally insert the {agent_id} in the path. If it's omitted, then it returns the history of the default agent.
  2. The streamlit app session_state isn't being updated when I select a new agent, so the previous history messages will stay on the users screen even though they will no longer be visible to the agent. This is likely to confuse the user. I guess to provide persistence, we would need to query the history endpoint each time you change agents in case you're returning to an agent you talked with before.

I guess we would ideally provide some tests for this functionality as well, since the tests are passing even though the user interaction behavior will be broken.

@jinhyun95
Copy link
Author

So I think the simplified change creates a few problems, particularly in the interaction with the streamlit app (maybe we need to add tests for those)

1. The `/history` endpoint just looks naively for the thread_id, so I think it will always return nothing since it doesn't match the format threads are being saved in. To resolve this, we might use the same pattern used by `/invoke` and `/stream` where you can optionally insert the `{agent_id}` in the path. If it's omitted, then it returns the history of the default agent.

2. The streamlit app session_state isn't being updated when I select a new agent, so the previous history messages will stay on the users screen even though they will no longer be visible to the agent. This is likely to confuse the user. I guess to provide persistence, we would need to query the history endpoint each time you change agents in case you're returning to an agent you talked with before.

I guess we would ideally provide some tests for this functionality as well, since the tests are passing even though the user interaction behavior will be broken.

While working on (1), I noticed that chatbot agent's state only retains the latest chat message due to the agent's lack of explicit state management.
Would you mind if I replace the agent with that with MessageState?

@JoshuaC215
Copy link
Owner

Happy to review but it should be a separate PR

"""
run_id = uuid4()
thread_id = user_input.thread_id or str(uuid4())
thread_id = f"{user_input.thread_id or str(uuid4())}-{agent_id}"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thread_id must be uuid4 by langgraph api spec and langchain agent protocol.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants