Skip to content

TOW-771 - logs being truncated - #120

Merged
socksy merged 5 commits into
developfrom
feature/tow-771-cli-logs-are-truncated
Oct 26, 2025
Merged

TOW-771 - logs being truncated #120
socksy merged 5 commits into
developfrom
feature/tow-771-cli-logs-are-truncated

Conversation

@socksy

Copy link
Copy Markdown
Contributor

Looks like the server never closes the SSE stream, so it's up to us in the client (backend change forthcoming...). Presumably not a big deal on the web frontend because it's just constantly open, for now I'm adding a 5 second timeout after we detect the run is complete which should be plenty of time to get any remaining logs, and hopefully not so long that it makes people annoyed that it takes too long for the CLI to be responsive after setting a run in motion.

@socksy
socksy requested a review from CopilotOctober 20, 2025 22:17

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses an issue where logs were being truncated due to the SSE stream not being properly closed. The solution adds client-side timeout handling to ensure logs that arrive after a run completes are still captured.

  • Adds a 5-second timeout after run completion to allow remaining logs to arrive
  • Implements specialized test infrastructure to verify logs arriving after run completion
  • Refactors mock API server log handling to be more maintainable with reusable helper functions

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

FileDescription
tests/mock-api-server/main.pyRefactors log generation into reusable helpers and adds test case for logs arriving after run completion
tests/integration/features/steps/mcp_steps.pyAdds step definition for creating named hello world applications
tests/integration/features/cli_runs.featureAdds test scenario to verify logs arriving after run completion are displayed

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +456 to +462
NORMAL_LOG_ENTRIES = [
(1, "Starting application...", "2025-08-22T12:00:00Z"),
(2, "Hello, World!", "2025-08-22T12:00:01Z"),
(3, "Application completed successfully", "2025-08-22T12:00:02Z"),
]


CopilotAIOct 20, 2025

Copy link

Choose a reason for hiding this comment

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

[nitpick] The hardcoded timestamps use dates in the future (2025-08-22). Consider using relative timestamps or dynamic timestamp generation to avoid confusion and make the test data more maintainable.

Suggested change
NORMAL_LOG_ENTRIES= [
(1, "Starting application...", "2025-08-22T12:00:00Z"),
(2, "Hello, World!", "2025-08-22T12:00:01Z"),
(3, "Application completed successfully", "2025-08-22T12:00:02Z"),
]
def_make_normal_log_entries():
base_time=datetime.datetime.utcnow()
return [
(1, "Starting application...", (base_time+datetime.timedelta(seconds=0)).strftime("%Y-%m-%dT%H:%M:%SZ")),
(2, "Hello, World!", (base_time+datetime.timedelta(seconds=1)).strftime("%Y-%m-%dT%H:%M:%SZ")),
(3, "Application completed successfully", (base_time+datetime.timedelta(seconds=2)).strftime("%Y-%m-%dT%H:%M:%SZ")),
]
NORMAL_LOG_ENTRIES=_make_normal_log_entries()

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

[nitpick] The hardcoded timestamps use dates in the future (2025-08-22).

Copilot is living in the past 😱

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@sammutisammuti left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Comment threadcrates/tower-cmd/src/run.rs Outdated
Base automatically changed from ben/stdio-mcp to developOctober 21, 2025 17:20

@bradhebradhe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, I think this is a decent solution. There's a lot of complexity in here that could be solved with the control plane issuing a EOF message 😄

@socksy
socksy merged commit 4c99d10 into developOct 26, 2025
6 checks passed
@socksy
socksy deleted the feature/tow-771-cli-logs-are-truncated branch October 26, 2025 16:53
bradhe pushed a commit that referenced this pull request Nov 2, 2025
Sign up for freeto 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.

4 participants

@socksy@bradhe@sammuti