Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b66b3551e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let escaped_message = request.message.replace('"', "\\\""); | ||
| let cmd = format!("git commit -m \"{}\"", escaped_message); |
There was a problem hiding this comment.
Avoid shell-interpolating user commit messages
The /api/git/commit handler builds a shell command string from untrusted request.message and only escapes ", which still allows shell expansion inside double quotes (for example $(...) or backticks). Because execute_shell_command runs via shell -c, a crafted commit message can execute arbitrary commands on the host instead of just creating a git commit.
Useful? React with 👍 / 👎.
| let title = if message.len() > 100 { | ||
| format!("{}...", &message[0..100]) |
There was a problem hiding this comment.
Truncate task titles on character boundaries
This truncation slices the message by byte index (&message[0..100]), which panics when byte 100 is not a UTF-8 character boundary. A long non-ASCII input (for example emoji or CJK text) will crash task submission instead of returning a normal API error.
Useful? React with 👍 / 👎.
| if let Some(mut task) = store.get_task(task_id).await { | ||
| task.complete(); | ||
| store.update_task(task.clone()).await; | ||
| let event = TaskEvent::completed(); | ||
| store.append_event(task_id, event.clone()).await; |
There was a problem hiding this comment.
Skip completed event emission for cancelled tasks
The execution path always appends a completed event after the stream loop, even if another request already cancelled the task. Since task.complete() is a no-op for cancelled status, this can leave status as cancelled while still broadcasting completed, causing clients to treat cancelled runs as successful completions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds a new paws_server HTTP API intended to support a stateless WebUI by introducing task-based orchestration, SSE streaming, and various read-only/configuration endpoints.
Changes:
- Introduces a new
paws_servercrate with Axum routing, task lifecycle management, in-memory task/event storage, and SSE streaming. - Adds a
servecommand topaws_mainto run the HTTP server. - Updates several
paws_domaintypes to beserde-serializable for API responses, and adds an API spec + demo script.
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/paws_server/src/task/store.rs | Adds task domain types plus an in-memory task/event store with tests. |
| crates/paws_server/src/task/mod.rs | Exposes task manager/store types from the new module. |
| crates/paws_server/src/task/manager.rs | Implements background task submission/execution and event emission. |
| crates/paws_server/src/server.rs | Sets up Axum router, routes, middleware, and shared app state. |
| crates/paws_server/src/lib.rs | Wires the new server crate modules and re-exports core types. |
| crates/paws_server/src/handlers/tasks.rs | Implements task CRUD-ish endpoints and event retrieval APIs. |
| crates/paws_server/src/handlers/sse.rs | Implements SSE streaming endpoints for task events (incl. resumable). |
| crates/paws_server/src/handlers/mod.rs | Exposes handler modules for router wiring. |
| crates/paws_server/src/handlers/git.rs | Adds git status/diff and commit endpoints for WebUI. |
| crates/paws_server/src/handlers/files.rs | Adds a file discovery/list endpoint for WebUI. |
| crates/paws_server/src/handlers/conversations.rs | Adds conversation CRUD endpoints used by the UI. |
| crates/paws_server/src/handlers/config.rs | Adds health/env/config/auth/MCP endpoints used by the UI. |
| crates/paws_server/src/events.rs | Adds TaskEvent model + broadcaster (per-task broadcast channels). |
| crates/paws_server/src/error.rs | Adds consistent JSON error responses for the HTTP API. |
| crates/paws_server/Cargo.toml | Declares dependencies for the new Axum-based server crate. |
| crates/paws_repo/src/database/migrations/2026-02-09-000000_add_conversation_state/up.sql | Adds a state column to conversations. |
| crates/paws_repo/src/database/migrations/2026-02-09-000000_add_conversation_state/down.sql | Drops the new state column on rollback. |
| crates/paws_main/src/ui.rs | Implements TopLevelCommand::Serve execution by running the server. |
| crates/paws_main/src/cli.rs | Adds a serve --port CLI command. |
| crates/paws_main/Cargo.toml | Adds dependency on paws_server. |
| crates/paws_domain/src/provider.rs | Makes AnyProvider serializable for API responses. |
| crates/paws_domain/src/chat_response.rs | Makes chat response types serializable/deserializable for API/SSE. |
| crates/paws_domain/src/auth/auth_context.rs | Makes auth context types serializable/deserializable for API. |
| crates/paws_domain/src/app_config.rs | Makes InitAuth serializable for API responses. |
| crates/paws_domain/src/agent.rs | Makes Agent serializable for API responses. |
| api_demo.sh | Adds a curl-based script demonstrating the new API workflow. |
| Cargo.toml | Adds paws_server to workspace dependencies. |
| API_SPEC.md | Adds a detailed HTTP API specification for the new server. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Use the first 100 chars of message as title | ||
| let title = if message.len() > 100 { | ||
| format!("{}...", &message[0..100]) | ||
| } else { | ||
| message.clone() |
There was a problem hiding this comment.
Slicing message[0..100] can panic for non-ASCII/UTF-8 input if byte index 100 is not on a char boundary. Consider building the title using .chars().take(100) (or a grapheme-aware approach if you care about user-visible characters) and then appending ....
| // Use the first 100 chars of message as title | |
| let title = if message.len() > 100 { | |
| format!("{}...", &message[0..100]) | |
| } else { | |
| message.clone() | |
| // Use the first 100 chars of message as title (truncate safely on char boundaries) | |
| let title = { | |
| let mut chars = message.chars(); | |
| let mut truncated: String = chars.by_ref().take(100).collect(); | |
| if chars.next().is_some() { | |
| truncated.push_str("..."); | |
| truncated | |
| } else { | |
| message.clone() | |
| } |
| match api.chat(chat_request).await { | ||
| Ok(mut stream) => { | ||
| while let Some(result) = futures::StreamExt::next(&mut stream).await { | ||
| match result { | ||
| Ok(response) => { | ||
| // Check for completion | ||
| let is_complete = | ||
| matches!(response, paws_domain::ChatResponse::TaskComplete); | ||
|
|
||
| // Broadcast the response | ||
| let event = TaskEvent::message(response); | ||
| store.append_event(task_id, event.clone()).await; | ||
| broadcaster.broadcast(task_id, event).await; | ||
|
|
||
| if is_complete { | ||
| break; | ||
| } | ||
| } | ||
| Err(e) => { | ||
| error!(task_id = %task_id, error = %e, "Stream error"); | ||
| let event = TaskEvent::error(e.to_string()); | ||
| store.append_event(task_id, event.clone()).await; | ||
| broadcaster.broadcast(task_id, event).await; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Mark task as completed | ||
| if let Some(mut task) = store.get_task(task_id).await { | ||
| task.complete(); | ||
| store.update_task(task.clone()).await; | ||
| let event = TaskEvent::completed(); | ||
| store.append_event(task_id, event.clone()).await; | ||
| broadcaster.broadcast(task_id, event).await; | ||
| info!(task_id = %task_id, "Task completed"); | ||
| } |
There was a problem hiding this comment.
If the chat stream yields an Err(e), the code emits a transient TaskEvent::error(...) but still proceeds to mark the task Completed after the loop ends. This can produce conflicting terminal states/events (error + completed). Consider treating stream errors as terminal failure (mark task Failed + emit failed) or track an errored flag and avoid complete() when any error occurred.
| pub async fn cancel(&self, id: TaskId) -> anyhow::Result<()> { | ||
| if let Some(mut task) = self.store.get_task(id).await { | ||
| if task.status.is_terminal() { | ||
| return Err(anyhow::anyhow!("Task already completed")); | ||
| } | ||
|
|
||
| task.cancel(); | ||
| self.store.update_task(task).await; | ||
| let event = TaskEvent::cancelled(); | ||
| self.store.append_event(id, event.clone()).await; | ||
| self.broadcaster.broadcast(id, event).await; | ||
| info!(task_id = %id, "Task cancelled"); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The cancel endpoint only updates stored state and broadcasts a cancelled event; it does not actually stop the spawned background task. As a result, cancelled tasks can still keep streaming/broadcasting and may later be marked Completed by spawn_execution. Consider introducing a cancellation mechanism (e.g., tokio_util::sync::CancellationToken, storing JoinHandle and calling abort(), and/or checking a cancelled flag inside the chat loop) so cancellation affects execution.
| pub async fn stream_task_events_resumable( | ||
| State(state): State<AppState>, | ||
| Path(id): Path<String>, | ||
| axum::extract::Query(query): axum::extract::Query<StreamSinceQuery>, | ||
| ) -> Result<impl IntoResponse, AppError> { | ||
| // Validate task ID before parsing | ||
| if id == "undefined" || id.is_empty() { | ||
| return Err(AppError::bad_request( | ||
| "Invalid task ID: task ID is undefined or empty. Please create a task first using POST /api/tasks", | ||
| )); | ||
| } | ||
|
|
||
| let task_id: TaskId = id.parse() | ||
| .map_err(|e: uuid::Error| AppError::bad_request(format!("Invalid task ID '{}': {}. Task ID must be a valid UUID. Please create a task first using POST /api/tasks", id, e)))?; | ||
|
|
||
| let since = query.since.unwrap_or(0); | ||
|
|
||
| // Get missed events first | ||
| let missed_events = state.task_store.get_events_since(task_id, since).await; | ||
|
|
||
| // Subscribe to live events | ||
| let receiver = state.broadcaster.subscribe(task_id).await; | ||
| let live_stream = BroadcastStream::new(receiver); |
There was a problem hiding this comment.
Unlike stream_task_events, the resumable endpoint does not verify that the task exists before subscribing. For an unknown task_id, this will create/subscribe to a channel and return an SSE stream that may hang indefinitely. Consider checking state.task_store.get_task(task_id).await and returning a 404 if absent (consistent with the non-resumable stream endpoint).
| // Then commit | ||
| // Escape quotes in message to prevent shell injection/breaking | ||
| let escaped_message = request.message.replace('"', "\\\""); | ||
| let cmd = format!("git commit -m \"{}\"", escaped_message); | ||
|
|
||
| let output = state | ||
| .api | ||
| .execute_shell_command(&cmd, PathBuf::from(".")) | ||
| .await?; |
There was a problem hiding this comment.
Building a shell command string using user-provided input is still vulnerable to command injection or shell metacharacter issues (escaping \" alone is not sufficient; e.g., $(), backticks, newlines, etc.). Prefer an API that passes an argv array to Command (no shell), or update execute_shell_command usage to a safe structured command execution method if available.
| /// Ensures a broadcast channel exists for a task. | ||
| /// | ||
| /// This is useful to create the channel before any events are broadcast, | ||
| /// so that subsequent subscribers can receive events from the buffer. | ||
| pub async fn ensure_channel(&self, task_id: TaskId) { | ||
| let mut channels = self.channels.write().await; | ||
| channels | ||
| .entry(task_id) | ||
| .or_insert_with(|| broadcast::channel(self.capacity).0); | ||
| } |
There was a problem hiding this comment.
The doc comment implies tokio::sync::broadcast will buffer past events for subscribers that connect later. New broadcast subscribers do not receive messages sent before subscription. Since you already persist events in TaskStore, consider updating the comment to clarify that persistence/replay comes from the store, and ensure_channel only prevents broadcasts from being dropped due to a missing sender.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| async fn test_event_log() { | ||
| let log = EventLog::new(); | ||
| let task_id = TaskId::new(); | ||
|
|
||
| log.append(task_id, TaskEvent::started()).await; | ||
| // The rest of this test relied on methods we just removed | ||
| // since EventLog is now just a helper struct if used at all | ||
| // or we can remove EventLog entirely if it's not used by Broadcaster | ||
| // anymore. | ||
| } | ||
| } |
There was a problem hiding this comment.
This test currently has no assertions and effectively doesn't validate any behavior. Either add an assertion by exposing a minimal read method for EventLog (even test-only), or remove the test if EventLog is intentionally a write-only helper.
| // Validate task ID before parsing | ||
| if id == "undefined" || id.is_empty() { | ||
| return Err(AppError::bad_request( | ||
| "Invalid task ID: task ID is undefined or empty. Please create a task first using POST /api/tasks", | ||
| )); | ||
| } | ||
|
|
||
| let task_id: TaskId = id.parse() | ||
| .map_err(|e: uuid::Error| AppError::bad_request(format!("Invalid task ID '{}': {}. Task ID must be a valid UUID. Please create a task first using POST /api/tasks", id, e)))?; |
There was a problem hiding this comment.
The same id == \"undefined\" || id.is_empty() check and TaskId parse + error mapping is duplicated across multiple handlers (get task, cancel, events, events/since, SSE). Consider centralizing this into a small helper (e.g., fn parse_task_id(id: &str) -> Result<TaskId, AppError>) to keep the handlers consistent and easier to update.
| Get the currently active agent ID. | ||
|
|
||
| ```http | ||
| GET /api/active-agent |
There was a problem hiding this comment.
The API spec documents active-agent endpoints under /api/active-agent and /api/active-agent (POST), but the router in crates/paws_server/src/server.rs exposes them under /api/config/active-agent. Please align the documented paths with the implemented routes (or update the routes if the spec is the intended contract).
| GET /api/active-agent | |
| GET /api/config/active-agent |
|
@copilot https://github.com/apps/copilot-pull-request-reviewer AI crates/paws_server/src/handlers/tasks.rs https://github.com/apps/copilot-pull-request-reviewer AI The same id == "undefined" || id.is_empty() check and TaskId parse + error mapping is duplicated across multiple handlers (get task, cancel, events, events/since, SSE). Consider centralizing this into a small helper (e.g., fn parse_task_id(id: &str) -> Result<TaskId, AppError>) to keep the handlers consistent and easier to update. crates/paws_server/src/events.rs https://github.com/apps/copilot-pull-request-reviewer AI The doc comment implies tokio::sync::broadcast will buffer past events for subscribers that connect later. New broadcast subscribers do not receive messages sent before subscription. Since you already persist events in TaskStore, consider updating the comment to clarify that persistence/replay comes from the store, and ensure_channel only prevents broadcasts from being dropped due to a missing sender. |
|
@manthanabc I've opened a new pull request, #91, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Initial plan * Fix error handling and extract task ID validation helper Co-Authored-By: Paws <noreply@pawscode.dev> Co-authored-by: manthanabc <48511543+manthanabc@users.noreply.github.com> * Extract parse_task_id to shared utils module Co-Authored-By: Paws <noreply@pawscode.dev> Co-authored-by: manthanabc <48511543+manthanabc@users.noreply.github.com> * Add safety comment for unwrap usage Co-Authored-By: Paws <noreply@pawscode.dev> Co-authored-by: manthanabc <48511543+manthanabc@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: manthanabc <48511543+manthanabc@users.noreply.github.com>
|
@manthanabc I've opened a new pull request, #93, to work on those changes. Once the pull request is ready, I'll request review from you. |
- Add get_conversation_summaries method to API trait and implementations - Add new /api/conversations/summaries endpoint for lightweight data - Update UI to use summaries endpoint instead of full conversations - Conversations now load without context/messages, reducing payload size - Full conversation data loaded only when clicking on a conversation Backend changes: - Add ConversationSummary type with id, title, metrics, metadata - Add get_all_conversation_summaries to ConversationRepository - Implement in ConversationRepositoryImpl using ConversationRecord - Add method to Services trait and implementations - Wire up through API layer and HTTP handlers UI changes: - Add listConversationSummaries method to API client - Update ConversationManager to use summaries endpoint - Full conversation still loaded via conversation cache on selection Co-Authored-By: Paws <noreply@pawscode.dev>
- Add get_provider_models method to API trait and implementations - Add /api/providers/:id/models endpoint for fetching models for a specific provider - Only URL providers can fetch models (template providers return empty list) - UI can now fetch models on-demand when provider is selected Co-Authored-By: Paws <noreply@pawscode.dev>
Resolved conflict in server.rs: - Kept /api/conversations/summaries endpoint from local changes - Merged formatting from remote branch Co-Authored-By: Paws <noreply@pawscode.dev>
a7daadd to
60d6447
Compare
No description provided.