fix tui approval hotkeys when question modal is open

[igun997]

Summary

• prioritize approval input routing over question modal routing when both states are active in TUI
• keep Ctrl+Y / Ctrl+N / Ctrl+U shortcuts functional for active approvals and add focused regression coverage
• stabilize tests by isolating git command env in autoresearch test helper and clearing provider/model env vars in doctor JSON CLI test

Test plan

• cargo test --workspace
• cargo clippy -- -D warnings (via pre-commit hook)
• cargo fmt -- --check (via pre-commit hook)

Fixes #38.

---

Summary by cubic

Fixes broken approval hotkeys in the TUI when the question modal is open and adds a consistent modal-based question picker. Also introduces dynamic skill invocation and management: an invoke_skill tool, supporting-file inlining, and nca skills subcommands.

• New Features

  • Modal question picker with arrow-key navigation; hides inline options when open and adds focused state handling in TUI.
  • invoke_skill tool to load full skill instructions on demand; registered in the supervisor and reflected in the system prompt.
  • Skill supporting-file inlining via Skill::expanded_body() (uses regex); shows active skill in subagent sidebar rows.
  • nca skills CLI: list/add/remove/update skills backed by a new skill_installer (GitHub or local sources, lock file management).

• Bug Fixes

  • Approval shortcuts (Ctrl+Y / Ctrl+N / Ctrl+U) always win over question modal input; central hotkey mapping and stale modal-state cleanup on resume. Fixes bug : issue when sub agent require approval #38.
  • Stop infinite tool retry loops after 3 consecutive failures of the same tool.
  • Stabilize tests by stripping GIT_* env and disabling host hooks in git tests, and clearing provider/model env vars in the doctor JSON CLI test.

^{Written for commit 7d4b5d3. Summary will update on new commits.}

[cubic-dev-ai]

9 issues found across 22 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="docs/superpowers/plans/2026-03-24-skill-supporting-files.md">

<violation number="1" location="docs/superpowers/plans/2026-03-24-skill-supporting-files.md:585">
P2: Verification commands use developer-specific absolute paths, making the plan non-portable and brittle for other contributors/CI.</violation>
</file>

<file name="docs/superpowers/specs/2026-03-24-question-modal-picker-design.md">

<violation number="1" location="docs/superpowers/specs/2026-03-24-question-modal-picker-design.md:53">
P2: The new spec claims the question modal intercepts key events before any other handler, but current TUI logic explicitly prioritizes approval hotkeys over the question modal. This documentation contradicts the implemented behavior and the PR’s intent, risking future regressions.</violation>
</file>

<file name="docs/superpowers/specs/2026-03-24-invoke-skill-tool-design.md">

<violation number="1" location="docs/superpowers/specs/2026-03-24-invoke-skill-tool-design.md:66">
P2: Spec defines two different success response contracts (wrapped message vs raw `expanded_body()`), creating avoidable implementation/test drift.</violation>
</file>

<file name="crates/core/src/agent.rs">

<violation number="1" location="crates/core/src/agent.rs:520">
P2: The infinite-loop guard emits an error event but exits with `break msg`, so the function returns `Ok(final_text)` instead of an error. This makes a tool-loop abort look like a successful turn and skips the normal assistant message update, creating inconsistent result semantics.</violation>
</file>

<file name="crates/core/src/skills.rs">

<violation number="1" location="crates/core/src/skills.rs:475">
P2: AGENTS.md skills can still fall back to Level 2/3 resolution because `is_likely_skill_subdir` is true for normal workspace roots, enabling reads from `workspace_root.parent()` and escaping the workspace boundary.</violation>

<violation number="2" location="crates/core/src/skills.rs:542">
P2: extract_file_references claims to return references in first-occurrence order, but it scans in three separate passes (./, then @, then backticks), so mixed syntaxes will be reordered vs. their textual appearance.</violation>
</file>

<file name="crates/cli/src/main.rs">

<violation number="1" location="crates/cli/src/main.rs:1403">
P2: `git_head_commit` errors are silently converted to `None` and then persisted, which can clear a previously stored commit hash. That causes future `skills update` runs to treat the skill as a local install and skip updates permanently after a transient failure.</violation>
</file>

<file name="crates/core/src/skill_installer.rs">

<violation number="1" location="crates/core/src/skill_installer.rs:234">
P1: Recursive copy follows symlinks, allowing untrusted skills to copy files from outside the source tree. Add a symlink check to avoid traversing or copying symlink targets.</violation>

<violation number="2" location="crates/core/src/skill_installer.rs:331">
P2: No post-sanitization collision check: two different skill directories that sanitize to the same name will overwrite each other and the lock entry, leading to nondeterministic installs.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

P1: Recursive copy follows symlinks, allowing untrusted skills to copy files from outside the source tree. Add a symlink check to avoid traversing or copying symlink targets.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/core/src/skill_installer.rs, line 234:

<comment>Recursive copy follows symlinks, allowing untrusted skills to copy files from outside the source tree. Add a symlink check to avoid traversing or copying symlink targets.</comment>

<file context>
@@ -0,0 +1,588 @@
+        let entry = entry.map_err(|e| e.to_string())?;
+        let src_path = entry.path();
+        let dst_path = dst.join(entry.file_name());
+        if src_path.is_dir() {
+            copy_dir_recursive(&src_path, &dst_path)?;
+        } else {
</file context>

[cubic-dev-ai]

P2: Verification commands use developer-specific absolute paths, making the plan non-portable and brittle for other contributors/CI.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At docs/superpowers/plans/2026-03-24-skill-supporting-files.md, line 585:

<comment>Verification commands use developer-specific absolute paths, making the plan non-portable and brittle for other contributors/CI.</comment>

<file context>
@@ -0,0 +1,603 @@
+
+```bash
+mkdir -p .nca/skills/test-skill
+cp /home/nst/Documents/superpower/skills/subagent-driven-development/SKILL.md .nca/skills/test-skill/
+cp /home/nst/Documents/superpower/skills/subagent-driven-development/implementer-prompt.md .nca/skills/test-skill/
+cp /home/nst/Documents/superpower/skills/subagent-driven-development/spec-reviewer-prompt.md .nca/skills/test-skill/
</file context>

[cubic-dev-ai]

P2: The new spec claims the question modal intercepts key events before any other handler, but current TUI logic explicitly prioritizes approval hotkeys over the question modal. This documentation contradicts the implemented behavior and the PR’s intent, risking future regressions.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At docs/superpowers/specs/2026-03-24-question-modal-picker-design.md, line 53:

<comment>The new spec claims the question modal intercepts key events before any other handler, but current TUI logic explicitly prioritizes approval hotkeys over the question modal. This documentation contradicts the implemented behavior and the PR’s intent, risking future regressions.</comment>

<file context>
@@ -0,0 +1,131 @@
+
+### Key Event Handling
+
+When `question_modal_open` is true, key events are intercepted before any other handler (same priority as existing pickers):
+
+| Key | Action |
</file context>

Suggested change

	When `question_modal_open` is true, key events are intercepted before any other handler (same priority as existing pickers):
	When `question_modal_open` is true, key events are intercepted before normal composer handling, but approval hotkeys still take precedence when an approval is active (same priority as existing pickers).

[cubic-dev-ai]

P2: Spec defines two different success response contracts (wrapped message vs raw expanded_body()), creating avoidable implementation/test drift.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At docs/superpowers/specs/2026-03-24-invoke-skill-tool-design.md, line 66:

<comment>Spec defines two different success response contracts (wrapped message vs raw `expanded_body()`), creating avoidable implementation/test drift.</comment>

<file context>
@@ -0,0 +1,111 @@
+1. Parse `skill_name` from `call.input`
+2. Call `SkillCatalog::discover()` with stored workspace root and skill directories
+3. Find matching skill by `command` field
+4. Return `skill.expanded_body()` as the tool result
+5. On no match, return error listing available skill commands
+
</file context>

[cubic-dev-ai]

P2: extract_file_references claims to return references in first-occurrence order, but it scans in three separate passes (./, then @, then backticks), so mixed syntaxes will be reordered vs. their textual appearance.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/core/src/skills.rs, line 542:

<comment>extract_file_references claims to return references in first-occurrence order, but it scans in three separate passes (./, then @, then backticks), so mixed syntaxes will be reordered vs. their textual appearance.</comment>

<file context>
@@ -394,6 +438,139 @@ fn parse_permission_mode_str(raw: &str) -> Option<PermissionMode> {
+    let mut refs = Vec::new();
+
+    // Pattern 1: ./path references
+    for m in DOT_SLASH_RE.find_iter(body) {
+        let path = m.as_str().to_string();
+        let key = path.trim_start_matches("./").to_string();
</file context>

[cubic-dev-ai]

P2: git_head_commit errors are silently converted to None and then persisted, which can clear a previously stored commit hash. That causes future skills update runs to treat the skill as a local install and skip updates permanently after a transient failure.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/cli/src/main.rs, line 1403:

<comment>`git_head_commit` errors are silently converted to `None` and then persisted, which can clear a previously stored commit hash. That causes future `skills update` runs to treat the skill as a local install and skip updates permanently after a transient failure.</comment>

<file context>
@@ -1274,6 +1329,120 @@ fn list_skills(config: &NcaConfig, workspace_root: &Path, json: bool) -> anyhow:
+            };
+
+            let tmp = git_clone_to_temp(&url).map_err(anyhow::Error::msg)?;
+            let new_commit = git_head_commit(tmp.path()).ok();
+
+            if new_commit.as_deref() == entry.commit.as_deref() {
</file context>

[cubic-dev-ai]

P2: No post-sanitization collision check: two different skill directories that sanitize to the same name will overwrite each other and the lock entry, leading to nondeterministic installs.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/core/src/skill_installer.rs, line 331:

<comment>No post-sanitization collision check: two different skill directories that sanitize to the same name will overwrite each other and the lock entry, leading to nondeterministic installs.</comment>

<file context>
@@ -0,0 +1,588 @@
+
+    let mut installed = Vec::new();
+    for (name, src_dir) in &filtered {
+        let safe_name = sanitize_skill_name(name)?;
+        let dest = target_dir.join(&safe_name);
+
</file context>