Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
PR works? Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
sounds reasonable Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
sounds good Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Improved readme
Function was returning 'success' for all non-failed cases, but the correct contract (enforced by tests) is: - all rules applied, no errors → 'ok' - some rules applied or errors present → 'partial_success' - no rules applied with errors → 'failed' map_command_status() already maps 'ok'/'partial_success' → 'success' for the outer CommandResult, so callers are unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
'ok'/'partial_success'/'failed' are detail statuses stored in result.result payload only. map_command_status() maps them to outer 'success'/'failed' which Stacker reads. Stacker behavior unchanged: partial_success still results in CommandStatus::Failed via has_errors check in report.rs:82. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…mpose
Hardcoded stacker.try.direct replaced with ${DASHBOARD_URL:-https://stacker.try.direct}
for both agent and compose-agent services.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR removes several previously hardcoded external asset URLs by introducing a unified templating/layout + local static assets, and adds new UI/API surfaces for marketplace browsing, dashboard linking, and agent registration.
Changes:
- Added a new Tera
base.htmllayout, refreshed templates (login/dashboard/error), and moved UI behavior to a new vanillastatic/js/app.js+ expandedstatic/css/style.css. - Added marketplace + dashboard linking routes/handlers in the local Axum API, plus a new CLI
registersubcommand andagent::registrationmodule. - Added a new
probe_endpointsStacker command and tightened firewall iptables comment sanitization.
Reviewed changes
Copilot reviewed 22 out of 24 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| templates/base.html | New shared layout (sidebar/topbar, static includes). |
| templates/index.html | Dashboard template refactor to use the shared layout + new styling. |
| templates/login.html | Login page refactor to shared layout and local assets. |
| templates/error.html | Error page refactor to shared layout and local styling. |
| templates/marketplace.html | New marketplace UI with deploy modal + filtering JS. |
| templates/link.html | New dashboard-linking UI and multi-step flow rendering. |
| static/css/style.css | Major UI styling refresh + design tokens. |
| static/css/controls.css | Reduced to a compatibility placeholder comment. |
| static/js/app.js | New JS for tabs, dropdowns, modals, sidebar behavior. |
| static/js/tab-controller.js | Removed legacy tab controller. |
| static/img/logo.webp | Added local logo asset. |
| src/comms/local_api.rs | Added marketplace + linking routes and handlers. |
| src/agent/registration.rs | New registration + linking client for Stacker endpoints. |
| src/agent/mod.rs | Exposes the new registration module. |
| src/main.rs | Banner adjustment, CLI about change, and new register subcommand. |
| src/commands/stacker.rs | Added probe_endpoints command (incl. port probing). |
| src/commands/firewall.rs | Sanitizes iptables rule comments and clarifies status logic. |
| docker-compose.yml | Makes DASHBOARD_URL configurable via env expansion. |
| config.json | Sample config tweak (apps_info emptied). |
| docs/AGENT_REGISTRATION_SPEC.md | Minor formatting alignment. |
| TODO.md | Updated marketplace/registration roadmap items. |
| README.md | Major documentation refresh and simplification. |
| Cargo.toml / Cargo.lock | Version bump to 0.1.5. |
| background: var(--color-bg-secondary); | ||
| border-radius: var(--radius-md); | ||
| border: 1px solid var(--color-border); | ||
| transition: border-color 0.2s; | ||
| } | ||
| .deployment-item:hover { | ||
| border-color: var(--color-primary); |
There was a problem hiding this comment.
The inline CSS uses --color-bg-secondary / --color-primary, but these tokens are not defined in static/css/style.css. This can break the deployment list styling. Update these to existing tokens (e.g., --color-neutral-bg and --color-accent) or add the missing variables.
| background: var(--color-bg-secondary); | |
| border-radius: var(--radius-md); | |
| border: 1px solid var(--color-border); | |
| transition: border-color 0.2s; | |
| } | |
| .deployment-item:hover { | |
| border-color: var(--color-primary); | |
| background: var(--color-neutral-bg); | |
| border-radius: var(--radius-md); | |
| border: 1px solid var(--color-border); | |
| transition: border-color 0.2s; | |
| } | |
| .deployment-item:hover { | |
| border-color: var(--color-accent); |
| <p style="color:var(--color-text-muted);font-size:0.85rem;margin-bottom:8px;"> | ||
| Don't have a TryDirect account? You can continue using Status Panel in standalone mode. | ||
| </p> | ||
| <a href="https://try.direct/register" target="_blank" class="btn btn-secondary btn-sm" style="margin-right:8px;"> |
There was a problem hiding this comment.
Links that use target="_blank" should include rel="noopener noreferrer" to prevent the newly opened page from accessing window.opener. Add the rel attribute to this external link (and any other target="_blank" links in these templates).
| <a href="https://try.direct/register" target="_blank" class="btn btn-secondary btn-sm" style="margin-right:8px;"> | |
| <a href="https://try.direct/register" target="_blank" rel="noopener noreferrer" class="btn btn-secondary btn-sm" style="margin-right:8px;"> |
| <a href="https://try.direct/register" target="_blank" class="btn btn-secondary btn-sm" style="margin-right:8px;"> | ||
| Create Account | ||
| </a> |
There was a problem hiding this comment.
The PR is titled "Remove hardcoded urls", but this template still hardcodes https://try.direct/register. If the goal is to make external URLs configurable, consider sourcing this from a config/env-driven base URL (similar to MARKETPLACE_URL/DASHBOARD_URL) or routing through an internal link.
| Json(json!({"error": format!("Marketplace returned {}", status)})).into_response() | ||
| } | ||
| Err(e) => { | ||
| Json(json!({"error": format!("Failed to reach marketplace: {}", e)})).into_response() |
There was a problem hiding this comment.
On upstream failures, this handler returns a JSON error body but still responds with HTTP 200. Consider returning an appropriate status code (e.g., propagate the upstream status for Ok(resp) non-success; use 502/504 for network/timeouts) so clients can reliably detect errors.
| Json(json!({"error": format!("Marketplace returned {}", status)})).into_response() | |
| } | |
| Err(e) => { | |
| Json(json!({"error": format!("Failed to reach marketplace: {}", e)})).into_response() | |
| let body = json!({ | |
| "error": format!("Marketplace returned {}", status), | |
| }); | |
| (status, Json(body)).into_response() | |
| } | |
| Err(e) => { | |
| let status = if e.is_timeout() { | |
| StatusCode::GATEWAY_TIMEOUT | |
| } else { | |
| StatusCode::BAD_GATEWAY | |
| }; | |
| let body = json!({ | |
| "error": format!("Failed to reach marketplace: {}", e), | |
| }); | |
| (status, Json(body)).into_response() |
| if let Err(e) = std::fs::remove_file(reg_path) { | ||
| error!("failed to remove registration: {}", e); |
There was a problem hiding this comment.
unlink_handler uses std::fs::remove_file inside an async handler, which can block the runtime. Prefer tokio::fs::remove_file (and handle errors with an appropriate response status if unlink is user-facing).
| if let Err(e) = std::fs::remove_file(reg_path) { | |
| error!("failed to remove registration: {}", e); | |
| if let Err(e) = tokio::fs::remove_file(reg_path).await { | |
| error!("failed to remove registration: {}", e); | |
| if state.with_ui { | |
| return ( | |
| StatusCode::INTERNAL_SERVER_ERROR, | |
| Html("Failed to unlink dashboard. Please try again later."), | |
| ) | |
| .into_response(); | |
| } else { | |
| return ( | |
| StatusCode::INTERNAL_SERVER_ERROR, | |
| Json(json!({"error": "failed to remove registration"})), | |
| ) | |
| .into_response(); | |
| } |
| async fn marketplace_page(State(state): State<SharedState>) -> impl IntoResponse { | ||
| if !state.with_ui { | ||
| return Json(json!({"error": "UI not enabled"})).into_response(); | ||
| } |
There was a problem hiding this comment.
marketplace_page is accessible in UI mode without validating the user's session, unlike home() which redirects to /login when the session cookie is missing/invalid. If Marketplace should be behind login, add the same session validation logic here (and similarly for the deploy API).
| <span class="material-icons-outlined">info</span> | ||
| <div> | ||
| No deployments found for your account. Deploy a stack from the | ||
| <a href="/marketplace" style="color:var(--color-primary);text-decoration:underline;">Marketplace</a> |
There was a problem hiding this comment.
This link uses var(--color-primary) but that CSS variable is not defined in static/css/style.css, so the link color may fall back unexpectedly. Use an existing token (e.g., --color-accent) or define --color-primary in the design tokens section.
| <a href="/marketplace" style="color:var(--color-primary);text-decoration:underline;">Marketplace</a> | |
| <a href="/marketplace" style="color:var(--color-accent);text-decoration:underline;">Marketplace</a> |
| let reg_path = std::path::Path::new("/etc/status-panel/registration.json"); | ||
| if reg_path.exists() { | ||
| if let Ok(data) = std::fs::read_to_string(reg_path) { | ||
| if let Ok(reg) = serde_json::from_str::<serde_json::Value>(&data) { | ||
| context.insert("linked", &true); | ||
| context.insert( | ||
| "agent_id", | ||
| ®.get("agent_id") | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or("unknown"), | ||
| ); | ||
| context.insert( | ||
| "dashboard_url", | ||
| ®.get("dashboard_url") | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or(""), | ||
| ); | ||
| return render_template(&state, "link.html", &context).into_response(); | ||
| } |
There was a problem hiding this comment.
link_page performs blocking filesystem I/O (Path::exists, std::fs::read_to_string) inside an async handler. This can block the Tokio runtime under load. Use tokio::fs equivalents or wrap the blocking work in tokio::task::spawn_blocking.
| let reg_path = std::path::Path::new("/etc/status-panel/registration.json"); | |
| if reg_path.exists() { | |
| if let Ok(data) = std::fs::read_to_string(reg_path) { | |
| if let Ok(reg) = serde_json::from_str::<serde_json::Value>(&data) { | |
| context.insert("linked", &true); | |
| context.insert( | |
| "agent_id", | |
| ®.get("agent_id") | |
| .and_then(|v| v.as_str()) | |
| .unwrap_or("unknown"), | |
| ); | |
| context.insert( | |
| "dashboard_url", | |
| ®.get("dashboard_url") | |
| .and_then(|v| v.as_str()) | |
| .unwrap_or(""), | |
| ); | |
| return render_template(&state, "link.html", &context).into_response(); | |
| } | |
| let reg_path = "/etc/status-panel/registration.json"; | |
| if let Ok(data) = tokio::fs::read_to_string(reg_path).await { | |
| if let Ok(reg) = serde_json::from_str::<serde_json::Value>(&data) { | |
| context.insert("linked", &true); | |
| context.insert( | |
| "agent_id", | |
| ®.get("agent_id") | |
| .and_then(|v| v.as_str()) | |
| .unwrap_or("unknown"), | |
| ); | |
| context.insert( | |
| "dashboard_url", | |
| ®.get("dashboard_url") | |
| .and_then(|v| v.as_str()) | |
| .unwrap_or(""), | |
| ); | |
| return render_template(&state, "link.html", &context).into_response(); |
| let result = tokio::process::Command::new("stacker") | ||
| .args([ | ||
| "deploy", | ||
| "--from", | ||
| &format!("/opt/stacker/stacks/{}/", stack_id), | ||
| ]) |
There was a problem hiding this comment.
stack_id is used directly to construct a local path (/opt/stacker/stacks/{stack_id}/). Add validation (allowlist characters / reject .. and path separators) to prevent path traversal and unexpected filesystem access when invoking stacker deploy.
| async fn get_container_ports(container_name: &str) -> Result<Vec<u16>> { | ||
| use tokio::process::Command; | ||
|
|
||
| macro_rules! stacker_test { | ||
| ($name:ident, $cmd_name:expr, $payload:expr, $variant:path) => { | ||
| #[test] | ||
| fn $name() { | ||
| let cmd = AgentCommand { | ||
| id: "cmd-test".into(), | ||
| command_id: "cmd-test".into(), | ||
| name: $cmd_name.into(), | ||
| params: $payload, | ||
| deployment_hash: Some("testhash".into()), | ||
| app_code: Some("testapp".into()), | ||
| }; | ||
| let parsed = parse_stacker_command(&cmd).unwrap(); | ||
| match parsed { | ||
| Some($variant(_)) => {} | ||
| _ => panic!("Did not parse {} command correctly", $cmd_name), | ||
| let output = Command::new("docker") | ||
| .args([ |
There was a problem hiding this comment.
get_container_ports() shells out to the docker CLI. This adds an operational dependency on the docker binary and can fail in minimal images even though the agent already uses Bollard. Prefer using the existing Bollard client to inspect container port bindings instead of invoking docker inspect.
- Validate stack_id to prevent path traversal in marketplace deploy - Return proper HTTP status codes (502/504) on marketplace proxy errors - Replace blocking std::fs with tokio::fs in link_page and unlink_handler - Fix deployStack() to show stack name instead of ID in confirmation modal - Add rel="noopener noreferrer" to target="_blank" links Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
No description provided.