feat(gateway): coding-agent UX — projects, shell mode, coding skill

[ilblackdragon]

Summary

Turns the web gateway into a coding-agent front-end:

• Projects: reuse the engine v2 Project (no new type) and extend it with a GitHubRepo newtype + typed metadata view (github_repo, default_branch) surfaced through EngineProjectInfo. Conversations bind to a project via conversations.metadata.project_id; the active-pointer lives as a workspace MemoryDoc at projects/_active.json.
• Conversation chrome: a new bar above #tab-chat shows project name · folder · branch (dirty dot) · PR chip. Populated by a TTL'd ProjectContextCache that shells out to git / gh through the dispatcher — the list endpoint never blocks on the network.
• `!`-shell mode: typing ! toggles a CSS badge and routes through POST /api/chat/send mode=shell. The backend resolves the thread's active project, dispatches shell with workdir = project.workspace_path, and broadcasts shell_command / shell_output SSE events. Commands + output persist as role=shell_command / role=shell_output messages so history replay rebuilds the turn and the next LLM turn sees them in context.
• Coding skill: new skills/coding-repo/SKILL.md guides branch/test/PR discipline (PRs target staging, never --no-verify, never force-push to main/staging). The skills parser gains activation.context.require_project_field so the skill only fires when the active project has a GitHub repo configured — non-gated callers are unaffected (default context admits every skill).

All gateway write paths route through ToolDispatcher::dispatch() (four new project-admin tools). Read paths are annotated dispatch-exempt: read aggregation per `.claude/rules/tools.md`.

Test plan

• cargo fmt --all
• cargo clippy --all --benches --tests --examples --all-features -- -D warnings — zero warnings
• cargo test -p ironclaw_skills --lib — 171 passing
• cargo test -p ironclaw --lib — 5170+ passing (one pre-existing telegram test flakes on test-order; passes in isolation before and after these changes)
• cargo test --test project_admin_integration --features libsql — 5 passing
• IRONCLAW_LIVE_TESTS=1 cargo test --test live_coding_flow --features libsql -- --ignored — 4 passing against a real nearai/ironclaw clone
• scripts/pre-commit-safety.sh — clean
• scripts/check_gateway_boundaries.py — no new back-edges
• Playwright: test_coding_project_flow.py — 5 scenarios (chrome render, shell-badge toggle, shell turn round-trip + reload, no-project 409, GitHubRepo HTTP-boundary validation). Runs against mock LLM; needs e2e venv + chromium (see tests/e2e/CLAUDE.md).
• Manual browser smoke: point at /data/illia/ironclaw5 via the manage-projects modal, set active, confirm chrome populates; type !cargo check, confirm shell turn renders and persists across reload.

Follow-ups

• Wire the live-tier PR-flow scenarios (Codex/feature parity pr hook #6, Onboarding: select bundled Telegram channel and auto-install #7 in tests/live/README.md) — LLM-driven coding-repo activation that opens a real draft PR and tears down.
• Port per-project-context hints into the engine v2 path (crates/ironclaw_engine) so context gating applies to both v1 and v2 skill selection.
• Surface the coding-agent UX in the Config tool / status line.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Turns the web gateway into a coding-agent front-end by adding project context, ! shell-mode dispatch + rendering, and context-gated coding skills.

Changes:

• Added project administration via ToolDispatcher (create/update/set-active/assign-thread) and surfaced per-thread project context to the UI.
• Implemented ! shell mode end-to-end: /api/chat/send mode=shell, SSE events, persistence, and history replay as shell turns.
• Added runtime skill activation context gating and introduced the coding-repo skill.

Reviewed changes

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

Show a summary per file

File	Description
tests/ws_gateway_integration.rs	Updates test gateway state init for new project_context_cache field.
tests/support/gateway_workflow_harness.rs	Updates harness state init for new project_context_cache field.
tests/project_admin_integration.rs	Adds integration tests for project metadata view, GitHubRepo validation, and workspace-path resolution.
tests/openai_compat_integration.rs	Updates test gateway state init for new project_context_cache field.
tests/oauth_greeting_integration.rs	Updates test gateway state init for new project_context_cache field.
tests/multi_tenant_integration.rs	Updates test gateway state init for new project_context_cache field.
tests/live_coding_flow.rs	Adds ignored live-tier Rust scenarios for project + git context + shell-mode behavior.
tests/live/README.md	Documents live-tier test contract and invocation.
tests/e2e/scenarios/test_coding_project_flow.py	Adds Playwright E2E scenarios for project chrome + shell mode + persistence + validation.
tests/e2e/CLAUDE.md	Registers the new E2E scenario in the test index doc.
src/tools/registry.rs	Adds registration for project-admin tools.
src/tools/builtin/project_admin.rs	Implements dispatcher-compliant tools for project create/update/set-active/assign-thread.
src/tools/builtin/mod.rs	Exposes the new project-admin tools from the builtin module.
src/main.rs	Wires ProjectContextCache into the gateway when tool dispatch is enabled.
src/channels/web/util.rs	Adds shell-turn reconstruction when building turns from persisted DB messages.
src/channels/web/types.rs	Adds ChatSendMode, thread project context DTO, and TurnInfo.shell payload.
src/channels/web/tests/multi_tenant.rs	Updates test state init for new project_context_cache field.
src/channels/web/test_helpers.rs	Updates test builder state init for new project_context_cache field.
src/channels/web/server.rs	Implements /api/chat/send shell-mode handling, adds project hints into message metadata, and includes project context in thread listing/new-thread responses.
src/channels/web/platform/ws.rs	Updates platform ws tests for new project_context_cache field.
src/channels/web/platform/state.rs	Adds project_context_cache to GatewayState.
src/channels/web/platform/router.rs	Adds routes for project create/update/active and thread project assignment.
src/channels/web/platform/project_context_cache.rs	Adds TTL cache that refreshes git/gh state via shell-tool dispatch.
src/channels/web/platform/mod.rs	Exposes the new project_context_cache module.
src/channels/web/mod.rs	Adds builder wiring for project_context_cache into the gateway channel state.
src/channels/web/handlers/settings.rs	Updates handler tests for new project_context_cache field.
src/channels/web/handlers/engine.rs	Adds dispatcher-routed project mutation handlers and read-only project resolution helpers.
src/channels/web/handlers/chat.rs	Adds project context to thread list/new-thread responses.
src/bridge/router.rs	Extends EngineProjectInfo with workspace_path + typed metadata view; adds create/update project bridge funcs and conversation project binding.
src/bridge/mod.rs	Re-exports new bridge types/functions for project support.
src/app.rs	Registers project-admin tools alongside memory tools using the workspace resolver.
src/agent/dispatcher.rs	Builds SkillActivationContext from gateway-provided message metadata.
src/agent/agent_loop.rs	Plumbs activation context into skill selection (prefilter_skills_with_context).
skills/coding-repo/SKILL.md	Adds the new coding-repo skill, gated on having a GitHub-backed project.
crates/ironclaw_skills/src/types.rs	Adds activation context criteria and runtime SkillActivationContext.
crates/ironclaw_skills/src/selector.rs	Adds context-aware prefiltering and tests for context gating behavior.
crates/ironclaw_skills/src/lib.rs	Re-exports new context types and new selector entrypoint.
crates/ironclaw_gateway/static/style.css	Adds project chrome + modal styles and shell-mode turn/badge styling.
crates/ironclaw_gateway/static/app.js	Implements Project UI (chrome + modal), shell-mode input routing, SSE rendering, and history shell-turn rendering.
crates/ironclaw_common/src/lib.rs	Re-exports GitHubRepo newtype and error.
crates/ironclaw_common/src/identity.rs	Adds GitHubRepo newtype with validation + tests.
crates/ironclaw_common/src/event.rs	Adds shell_command / shell_output SSE event variants.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

refresh_pr runs gh pr view --json number,title,url,state, but parse_gh_pr relies on a draft flag. As written, the cache can never detect drafts because the command doesn't request the draft field. Include the draft field in the --json list (e.g., isDraft) so draft PRs are represented correctly in PrState.

Suggested change

	"gh pr view --json number,title,url,state",
	"gh pr view --json number,title,url,state,isDraft",

[Copilot]

parse_gh_pr deserializes a field named is_draft, but gh's JSON uses camelCase (isDraft). Without a #[serde(rename = "isDraft")] (and requesting that field in the command), row.is_draft will always be false and draft PRs will be misclassified as Open.

Suggested change

	#[serde(default)]
	#[serde(default, rename = "isDraft")]

[Copilot]

The docs claim the default SkillActivationContext "admits every skill", but satisfies(Some("github_repo")) returns false when the default booleans are false, so gated skills are filtered out by default. Please update the doc comment to match the implemented behavior (or adjust satisfies if the intended contract is truly 'admit by default').

Suggested change

	/// The default value (no project, no workspace path) admits every
	/// skill — callers with no project context pass it and no gate
	/// triggers. Callers that do know the active project populate the
	/// matching booleans so gated skills activate.
	/// The default value (no project, no workspace path) admits ungated
	/// skills, but does not satisfy recognised project-field gates such as
	/// `"github_repo"` or `"workspace_path"`. Unknown gate names still
	/// admit by default for forward compatibility. Callers that do know
	/// the active project populate the matching booleans so gated skills
	/// activate.

[Copilot]

This doc comment says name and description are required, but the implementation only requires name (description defaults via unwrap_or_default()). Please align the documentation with the actual behavior so callers know which fields are mandatory.

Suggested change

	/// `name` and `description` are required (via `fields.name` / `fields.description`).
	/// `name` is required (via `fields.name`). `description` is optional and
	/// defaults to an empty string when omitted.

[Copilot]

get() clones the current snapshot and spawn_stale_refreshes unconditionally tokio::spawns refresh tasks when fields are stale/missing. If multiple requests hit /api/chat/threads while entries are stale (or missing), this can fan out into many concurrent git/gh shell dispatches for the same project. Consider adding an in-flight guard (per project + field), or pessimistically updating the *_fetched_at timestamp before spawning, so repeated reads don't create a thundering herd of refresh jobs.

[Copilot]

shell_output rows are parsed assuming the content is a JSON object; if the persisted content is a JSON string (or invalid JSON), serde_json::from_str(...).unwrap_or(Value::Null) causes stdout/stderr to be dropped and exit_code to default to 0, which incorrectly marks the turn as Completed. Consider parsing the shell_output content with the same defensive logic as parse_shell_tool_output (object vs string), and use a non-zero/unknown exit code when parsing fails so the UI doesn't render a false success badge.

Suggested change

	let parsed: serde_json::Value =
	serde_json::from_str(&out.content).unwrap_or(serde_json::Value::Null);
	(
	parsed
	.get("stdout")
	.and_then(|v| v.as_str())
	.unwrap_or("")
	.to_string(),
	parsed
	.get("stderr")
	.and_then(|v| v.as_str())
	.unwrap_or("")
	.to_string(),
	parsed
	.get("exit_code")
	.and_then(|v| v.as_i64())
	.unwrap_or(0) as i32,
	parsed
	.get("truncated")
	.and_then(|v| v.as_bool())
	.unwrap_or(false),
	let (stdout, stderr, exit_code, truncated) =
	match serde_json::from_str::<serde_json::Value>(&out.content) {
	Ok(serde_json::Value::Object(parsed)) => (
	parsed
	.get("stdout")
	.and_then(|v| v.as_str())
	.unwrap_or("")
	.to_string(),
	parsed
	.get("stderr")
	.and_then(|v| v.as_str())
	.unwrap_or("")
	.to_string(),
	parsed
	.get("exit_code")
	.and_then(|v| v.as_i64())
	.map(|v| v as i32)
	.unwrap_or(1),
	parsed
	.get("truncated")
	.and_then(|v| v.as_bool())
	.unwrap_or(false),
	),
	Ok(serde_json::Value::String(stdout)) => (stdout, String::new(), 1, false),
	Ok(other) => (other.to_string(), String::new(), 1, false),
	Err(_) => (out.content.clone(), String::new(), 1, false),
	};
	(
	stdout,
	stderr,
	exit_code,
	truncated,

[gemini-code-assist]

Code Review

This pull request implements coding project management and a direct shell execution mode (via the ! prefix) in the gateway, introducing a validated GitHubRepo type, a TTL-bound ProjectContextCache for git status, and new administration tools. The review identifies several performance and architectural concerns, most notably the O(N) I/O-bound project resolution within thread list loops and inefficient frontend API usage for thread refreshes. Additionally, the feedback highlights a thundering herd risk in the cache refresh logic and the use of blocking synchronous filesystem operations within asynchronous functions, which could stall the runtime.

[gemini-code-assist]

This loop performs O(N) project resolution for every thread in the list. Each call to thread_project_context involves multiple I/O-bound operations: a database query for conversation metadata, a workspace file read for the active project pointer, and an engine store lookup. For users with many threads, this will significantly degrade the performance of the thread list endpoint. Since the project chrome is typically only displayed for the active thread, consider resolving the project context lazily or only for the current thread. Please ensure this optimization is justified by profiling data before implementation to avoid premature optimization.

References

1. Performance optimizations should be gated on profiling to ensure they address actual bottlenecks.
2. Avoid premature optimization; prioritize readability and maintainability for non-critical paths.

[gemini-code-assist]

Similar to the handler in chat.rs, this loop performs O(N) project resolution for every thread summary. This is highly inefficient as it triggers database queries and workspace reads for every thread in the list. This data is only used by the frontend for the currently active thread. Ensure this optimization is gated on profiling to confirm it is a performance bottleneck.

References

1. Performance optimizations should be gated on profiling to ensure they address actual bottlenecks.
2. Avoid premature optimization; prioritize readability and maintainability for non-critical paths.

[gemini-code-assist]

refreshCurrentThread fetches the entire thread list via /api/chat/threads just to find the project info for the current thread. This is inefficient, especially as the backend implementation of this endpoint is now significantly more expensive due to per-thread project resolution. Consider adding a dedicated endpoint for single-thread info or including the project context in the history response to avoid this O(N) frontend lookup.

[gemini-code-assist]

The spawn_stale_refreshes method lacks in-flight tracking. If multiple requests hit the cache for the same project simultaneously, it will spawn numerous redundant background tasks, each executing shell commands. Consider using a mechanism to track and deduplicate active refresh tasks for the same project ID to prevent a thundering herd problem. Note that adding such a caching layer or optimization should be gated on profiling per repository guidelines.

References

1. Performance optimizations, such as adding a caching layer, should be gated on profiling.

[gemini-code-assist]

Using std::fs::create_dir_all and Path::exists inside an async function blocks the executor thread. These should be replaced with tokio::fs counterparts (e.g., tokio::fs::create_dir_all and tokio::fs::try_exists) to avoid stalling the gateway's async runtime during shell-mode dispatch.

[Copilot]

Pull request overview

Copilot reviewed 66 out of 211 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

engineV2 is not defined in the shared bootstrap state (the global flag elsewhere is engineV2Enabled). As written, this will throw a ReferenceError on the first Mod+1..5 keypress (or always take the wrong branch if a different global happens to exist). Use the existing global (engineV2Enabled) consistently in this shortcut handler.

Suggested change

	const tabs = engineV2
	const tabs = engineV2Enabled

[Copilot]

The status payload fields used to enable engine-v2 UI are inconsistent (data.engine_v2 vs data.engine_v2_enabled). This can cause engineV2Enabled (and therefore tab visibility/activity strip behavior) to flip incorrectly depending on which field is actually present. Pick a single canonical field name from the API response and use it consistently for both activeWorkStore.setEngineV2Enabled(...) and engineV2Enabled = ....

[Copilot]

The doc/comments (and FromStr) say legacy "error" should be accepted as an alias for Failed, but serde deserialization for JobResultStatus will currently reject "error" on the wire. If older producers still emit "error", AppEvent::JobResult deserialization will fail. Fix by adding a serde alias on the Failed variant (e.g., #[serde(alias = "error")]), or by implementing a custom Deserialize that maps "error" → Failed.

[Copilot]

This unguarded getElementById(...).addEventListener(...) will throw at script-evaluation time if #skill-search-input is not present in the DOM (notably plausible given the new layout-level tab hiding and the fact that all split JS modules are concatenated and loaded globally). Use optional chaining (?.addEventListener) or a null check like other surfaces in this PR.

Suggested change

	document.getElementById('skill-search-input').addEventListener('keydown', function(e) {
	document.getElementById('skill-search-input')?.addEventListener('keydown', function(e) {

[Copilot]

This introduces a hard-coded English prompt string in an otherwise i18n-driven UI (the file already uses I18n.t(...) extensively). To keep localization and consistency, move both the prompt text and default value to i18n keys (and keep displayName as an interpolated param).

Suggested change

	var tokenName = prompt('Token name for ' + displayName + ':', 'api-token');
	var tokenName = prompt(
	I18n.t('users.tokenNamePrompt', { displayName: displayName }),
	I18n.t('users.defaultTokenName')
	);

[serrrfirat]

High Severity

This path accepts an arbitrary thread_id and then writes directly to that conversation without first checking conversation_belongs_to_user(...). Because resolve_thread_project() falls back to the caller's active/default project when it can't resolve thread-owned metadata, a user who guesses another conversation UUID can append shell_command / shell_output rows to someone else's thread.

The same root issue exists on project_assign_thread: please add a shared ownership guard before any thread-scoped mutation (add_conversation_message, set_conversation_project, etc.), and cover it with a multi-tenant caller-level test.

[serrrfirat]

High Severity

ToolDispatcher::dispatch("shell", ...) returns the shell tool's current wire shape {output, exit_code, ...}, but this cache still reads stdout. That leaves res.stdout empty on success, which in turn makes branch refreshes come back as None and dirty checks look clean even when the repo is dirty.

Please parse output here (matching parse_shell_tool_output() in the chat handler) and add a test against the real shell-tool payload shape so the cache and tool contract stay in sync.

[serrrfirat]

High Severity

This still keyword-scores every skill returned by __list_skills__() with no equivalent of v1's prefilter_skills_with_context(...). As a result, a skill like coding-repo can still activate on engine v2 threads that have no github_repo project field set, even though the new manifest contract says it should be gated out.

Please port the activation.context.require_project_field filtering into the v2 selector before scoring, otherwise v1 and v2 will diverge on exactly the project-scoping rule this PR is introducing.

[serrrfirat]

High Severity

This file now binds two change listeners to the same #image-file-input: the older one above calls handleImageFiles(...), while this one calls handleAttachmentFiles(...). For an image selection/paste, the same file gets staged into both stagedImages and stagedAttachments, and sendMessage() later submits both body.images and body.attachments.

That silently double-sends the same image to the backend/model. Please collapse the upload path to a single staging pipeline and add a regression test that verifies one picked image produces exactly one uploaded attachment payload.

[Copilot]

Pull request overview

Copilot reviewed 69 out of 69 changed files in this pull request and generated 8 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

parse_shell_output is reading stdout, but the shell tool returns { output, exit_code, … } (see src/tools/builtin/shell.rs). This makes branch/dirty/PR always compute from an empty string. Parse output (or support both output and stdout for compatibility).

[Copilot]

spawn_stale_refreshes spawns a new task on every call when a field is stale, with no in-flight dedupe per (project_id, field). Under /api/chat/threads polling this can create a thundering herd of git/gh calls. Consider tracking per-field refresh-in-progress (or last_refresh_started_at) to avoid concurrent duplicates.

[Copilot]

The doc comment for resolve_thread_project says precedence ends with None when there’s no override and no active pointer, but the implementation falls back to a "default" project. This changes the API semantics (and likely prevents the documented 409 "no project" shell-mode path). Either remove the fallback or update the docs/tests and error messaging to match the intended behavior.

[Copilot]

When a shell_command has no following shell_output, this code sets exit_code to 0 and marks the turn Completed, which is misleading (it’s an incomplete/failed turn). Consider using a sentinel exit_code (e.g. -1) and setting state to Failed/InProgress when the output row is missing.

[Copilot]

This module doc says the test "always cleans up" by closing PRs and pruning branches, but later notes the PR is intentionally left open and requires a manual cleanup script. Please make the doc and behavior consistent (ideally ensure teardown happens automatically, since this test mutates real GitHub state).

[Copilot]

use crate::tools::builtin::memory::WorkspaceResolver; appears unused in this module (no other references). This will trigger an unused-import warning/error under -D warnings; please remove it or use it explicitly.

[Copilot]

get_engine_project(&pid.0.to_string(), …).await borrows a temporary String across an .await, which won’t compile (temporary doesn’t live long enough). Convert once into a local String (or change the bridge API to accept ProjectId) before awaiting.

[Copilot]

Same temporary-borrow-across-.await issue here: get_engine_project(&pid.0.to_string(), …).await won’t compile. Store pid.0.to_string() in a local variable (or pass a typed ProjectId through the bridge) before awaiting.

[serrrfirat]

Paranoid Architect Review — Request Changes

2 Critical, 5 High, 5 Medium findings. This is the highest-risk PR in the batch — 6K+ lines touching gateway, engine, TUI, and skills with two directly exploitable security issues.

Critical

• XSS via attribute injection in project edit form (escapeHtml() doesn't escape quotes — 5 <input value="..."> constructions vulnerable)
• TUI shell mode bypasses ToolDispatcher — no approval checks, no audit trail, no safety pipeline

High

• Arbitrary directory traversal: workspace_path accepts any PathBuf with zero validation — user can set /etc and run shell commands there
• LLM metadata key injection: thread_metadata_set allows LLM to overwrite project_id, redirecting shell commands to a different workspace
• Unsanitized metadata in LLM prompt: Thread metadata injected as raw JSON into system prompt — prompt injection vector
• Confused deputy: Shell dispatch uses thread-requesting user_id — potential cross-user action in multi-tenant
• No input length limit on shell command content before dispatch

Medium

• parse_shell_output reads stdout key but shell tool returns output — cache will never populate
• Caller-supplied env vars can override safe PATH/LD_PRELOAD whitelist
• from_trusted + transparent serde allows unvalidated GitHubRepo to persist
• No concurrency guard on shell dispatch — concurrent commands interleave and garble history
• IDOR: resolve_thread_project reads conversation metadata by UUID without user ownership check

Blocking before merge

1. Fix the XSS: escapeHtml() must escape " and ' for attribute contexts
2. Route TUI shell through ToolDispatcher::dispatch()
3. Validate workspace_path (must be under ~/.ironclaw/ or project workspace root)
4. Add allow-list for thread_metadata_set writable keys (block project_id, source_channel, etc.)
5. Add content length cap on shell commands

[serrrfirat]

Paranoid Architect Review — Request Changes

2 Critical, 5 High, 5 Medium findings. This is the highest-risk PR in the batch — 6K+ lines touching gateway, engine, TUI, and skills with two directly exploitable security issues.

Critical

• XSS via attribute injection in project edit form — escapeHtml() doesn't escape quotes. 5 <input value="...escapeHtml(x)..."> constructions allow attribute breakout. A project name like " onfocus="alert(document.cookie) injects arbitrary event handlers.
• TUI shell mode bypasses ToolDispatcher — calls tool.execute() directly, skipping the approval pipeline, audit trail, safety layer, and rate limiter. Gateway path is correct.

High

• Arbitrary directory traversal: workspace_path accepts any PathBuf with zero validation — user can set /etc and run shell commands there.
• LLM metadata key injection: thread_metadata_set allows LLM to overwrite project_id, redirecting shell commands to a different workspace.
• Unsanitized metadata in LLM prompt: Thread metadata injected as raw JSON into system prompt — prompt injection vector via skill-written metadata values.
• Confused deputy: Shell dispatch uses thread-requesting user_id — potential cross-user action in multi-tenant.
• No input length limit on shell command content before dispatch.

Medium

• parse_shell_output reads stdout key but shell tool returns output — cache will never populate (bug).
• Caller-supplied env vars can override safe PATH/LD_PRELOAD whitelist in FilesystemBackend::shell.
• from_trusted + transparent serde allows unvalidated GitHubRepo to persist and potentially render unsafely.
• No concurrency guard on shell dispatch — concurrent commands interleave and garble history replay.
• IDOR: resolve_thread_project reads conversation metadata by UUID without user ownership check.

Blocking before merge

1. Fix XSS: escapeHtml() must escape " and ' for attribute contexts, or build DOM elements programmatically
2. Route TUI shell through ToolDispatcher::dispatch()
3. Validate workspace_path (must be under allowed workspace root)
4. Add allow-list for thread_metadata_set writable keys (block project_id, source_channel)
5. Add content length cap on shell commands
6. Fix parse_shell_output to read "output" instead of "stdout"

[serrrfirat]

Critical: XSS via HTML attribute injection. escapeHtml() uses the textContent/innerHTML DOM technique which escapes <, >, & but does NOT escape double quotes. This <input value="...escapeHtml(x)..."> construction is vulnerable to attribute breakout.

A project name like " onfocus="alert(document.cookie) injects arbitrary event handlers into 5 input fields in the edit form.

Fix: Either use a proper attribute escaper that also handles " and ', or build DOM elements programmatically with input.value = rawString instead of string interpolation.

[serrrfirat]

High: Arbitrary directory traversal. workspace_path is accepted as any arbitrary PathBuf with zero validation — no check that it's under ~/.ironclaw/, no rejection of sensitive directories (/etc, /root). A user can create a project with workspace_path="/etc" and then run !cat shadow in shell mode.

The shell tool's blocklist catches cat /etc/shadow but NOT cat shadow when the workdir is already /etc.

Fix: Validate that workspace_path resolves to a path under the allowed workspace root (e.g., ~/.ironclaw/workspaces/), or at minimum reject paths outside $HOME.

[serrrfirat]

Medium: Caller env vars override safe whitelist. This applies caller-supplied env vars AFTER setting safe vars (PATH, HOME, etc.), meaning a caller can inject PATH=/evil/bin or LD_PRELOAD=/evil.so.

Fix: Apply safe vars last (after caller env), or filter out security-critical keys from the caller's env map.

[Copilot]

Pull request overview

Copilot reviewed 74 out of 74 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The TUI shell-mode path executes the shell tool directly via Tool::execute, bypassing ToolDispatcher’s approval/audit pipeline. That means commands that are meant to be approval-gated (e.g. git reset --hard, git clean -f) can run without any approval checks. Route shell-mode through the dispatcher (if available here) or explicitly enforce requires_approval before executing.

Suggested change

	let out = tool
	.execute(serde_json::Value::Object(params), &ctx)
	let params = serde_json::Value::Object(params);
	if tool.requires_approval(&params) {
	return Err("shell command requires approval and must be executed through the approval pipeline".to_string());
	}
	let out = tool
	.execute(params, &ctx)

[Copilot]

parse_shell_output is looking for stdout, but the built-in shell tool returns { "output": ..., "exit_code": ... } (see src/tools/builtin/shell.rs). As a result, the project context cache will treat every command as having empty output and branch/dirty/PR probing won’t populate. Update this parser to read from output (and keep the string fallback if desired).

[Copilot]

get() can spawn duplicate refresh tasks for the same project/field when multiple requests arrive before the first refresh updates *_fetched_at (e.g. thread list + history in quick succession). This can lead to unbounded concurrent git/gh shell-outs and unnecessary load. Consider tracking per-project in-flight refreshes (or setting the fetched-at timestamp optimistically before spawning) so only one refresh per field runs at a time.

[Copilot]

When a shell_command message has no following shell_output row, this builds a turn with exit_code = 0 and state Completed, which is misleading (it’s more likely “pending/unknown” or a failed/crashed dispatch). Consider using a sentinel exit code (e.g. -1) and/or a distinct state when the output row is missing or unparseable so the UI doesn’t show false success.

[Copilot]

Pull request overview

Copilot reviewed 77 out of 77 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Unused import: WorkspaceResolver is imported but not referenced anywhere in this module. With -D warnings/clippy this will fail CI; please remove it (or use it if something is missing).

[Copilot]

refresh_pr runs gh pr view --json number,title,url,state, but parse_gh_pr expects a draft flag (is_draft). GitHub CLI uses isDraft and it is only present if requested via --json isDraft. As written, draft PRs will be reported as open in the chrome. Consider adding isDraft to the command and deserializing with #[serde(rename = "isDraft")].

[Copilot]

The doc comment for resolve_thread_project says precedence step 3 returns None (no project chrome), but the implementation always falls back to the user's default project. Please update the comment to match the actual behavior so callers and future maintainers don't rely on the wrong contract.