diff --git a/.github/workflows/smoke.yml b/.github/workflows/smoke.yml index c52c1b9a0..32093920e 100644 --- a/.github/workflows/smoke.yml +++ b/.github/workflows/smoke.yml @@ -176,7 +176,7 @@ jobs: name: Screenshot Regression (Linux) needs: publish runs-on: ubuntu-latest - timeout-minutes: 30 + timeout-minutes: 45 env: SMOKE_OLLAMA_MODEL: qwen2:0.5b diff --git a/.github/workflows/validate_docker_image.yml b/.github/workflows/validate_docker_image.yml index 24a1b52d1..af7f03b8f 100644 --- a/.github/workflows/validate_docker_image.yml +++ b/.github/workflows/validate_docker_image.yml @@ -46,7 +46,7 @@ jobs: validate-docker-build: name: Validate Docker Build runs-on: ubuntu-latest - timeout-minutes: 20 + timeout-minutes: 30 steps: - name: "Checkout" diff --git a/scripts/smoke/run-native-tape.sh b/scripts/smoke/run-native-tape.sh index 06656e70a..c18060eb0 100755 --- a/scripts/smoke/run-native-tape.sh +++ b/scripts/smoke/run-native-tape.sh @@ -116,19 +116,16 @@ collect_failure_artifacts() { echo " artifacts: $(ls "${ARTIFACT_DIR}" 2>/dev/null | tr '\n' ' ')" } -# Substitute placeholders in preamble. Sed delimiter is '|' since paths -# contain '/'. The substituted values are paths set by us, so escaping -# is minimal. -sed \ +# Substitute placeholders in both preamble and body. Sed delimiter is '|' +# since paths contain '/'. Concatenating both files before the sed pass +# means body tapes can use any token, not just the preamble. +cat "$preamble" "$body" | sed \ -e "s|__NETCLAW_HOME__|${NETCLAW_HOME}|g" \ -e "s|__NETCLAW_BIN_DIR__|${NETCLAW_BIN_DIR}|g" \ -e "s|__NETCLAW_DAEMON__|${NETCLAW_SMOKE_DAEMON}|g" \ -e "s|__TAPE_NAME__|${TAPE_NAME}|g" \ - "$preamble" > "$combined" - -# Append body. The body declares its own `Output ...`; last-write-wins -# on Output is fine in vhs. -cat "$body" >> "$combined" + -e "s|__NETCLAW_SMOKE_MCP_SERVER__|${NETCLAW_SMOKE_MCP_SERVER:-}|g" \ + > "$combined" echo "==> Running native tape: ${TAPE_NAME} (timeout=${TAPE_TIMEOUT_S}s)" echo " NETCLAW_BIN_DIR=${NETCLAW_BIN_DIR}" diff --git a/scripts/smoke/run-smoke.sh b/scripts/smoke/run-smoke.sh index f6065e0e9..8ebff13b1 100755 --- a/scripts/smoke/run-smoke.sh +++ b/scripts/smoke/run-smoke.sh @@ -73,12 +73,14 @@ FULL_SCENARIOS=("${LIGHT_SCENARIOS[@]}") # may emit several `Screenshot "/tmp/shot-.png"` directives. SHOT_FRAMES # is the full set of frame names the harness compares against baselines — it # MUST stay in sync with the Screenshot paths in those tapes. -SHOT_TAPES=(help wizard-screens provider-manager) +SHOT_TAPES=(help wizard-screens provider-manager mcp-permissions) SHOT_FRAMES=( help wizard-provider-picker wizard-security-posture provider-manager-empty + mcp-permissions-server-list + mcp-permissions-tool-grid ) usage() { diff --git a/src/Netclaw.Cli.Tests/Mcp/McpToolPermissionsPageTests.cs b/src/Netclaw.Cli.Tests/Mcp/McpToolPermissionsPageTests.cs index ad0602a01..723f60c7c 100644 --- a/src/Netclaw.Cli.Tests/Mcp/McpToolPermissionsPageTests.cs +++ b/src/Netclaw.Cli.Tests/Mcp/McpToolPermissionsPageTests.cs @@ -300,6 +300,34 @@ public async Task ToolGrid_RightArrowOnServerEnabledRow_TogglesServerAccess() Assert.NotEqual(wasBefore, vm.IsServerAllowedForSelectedAudience()); } + [Fact] + public async Task ToolGrid_ManyTools_HeaderRowsNotOverwrittenByScrollContent() + { + // Regression test for issue #1424: ScrollableContainerNode.Render ignores + // bounds.Y and writes at context (0,0). With enough tools the scroll container + // overwrites the server-info and audience rows. The fix wraps the container in + // a borderless PanelNode so it receives a properly-offset render context. + var (terminal, app, vm) = CreateHeadlessApp(out var input); + + // 15 tools: on a 40-row terminal the tool list starts at row ~10 without the + // bug fix; with the bug it writes at row 0 and overwrites every header row. + var tools = Enumerable.Range(1, 15).Select(i => $"tool-{i:00}").ToList(); + vm.InitializeForTests(new McpServerName("notion"), tools); + vm.SetSelectedAudienceForTests(TrustAudience.Personal); + + input.EnqueueKey(ConsoleKey.Q, false, false, true); + + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(10)); + await app.RunAsync(cts.Token); + + Assert.True(terminal.Contains("MCP Permissions"), + $"Expected page header. Screen:\n{terminal}"); + Assert.True(terminal.Contains("Audience"), + $"Expected 'Audience' row not overwritten by tool list. Screen:\n{terminal}"); + Assert.True(terminal.Contains("Server default"), + $"Expected 'Server default' row not overwritten by tool list. Screen:\n{terminal}"); + } + // ── Helpers ────────────────────────────────────────────────────────────── private (VirtualTerminal Terminal, TerminaApplication App, McpToolPermissionsViewModel Vm) diff --git a/src/Netclaw.Cli/Mcp/McpToolPermissionsPage.cs b/src/Netclaw.Cli/Mcp/McpToolPermissionsPage.cs index 2717c6a83..18c1edeea 100644 --- a/src/Netclaw.Cli/Mcp/McpToolPermissionsPage.cs +++ b/src/Netclaw.Cli/Mcp/McpToolPermissionsPage.cs @@ -177,7 +177,15 @@ private ILayoutNode BuildToolGrid() .WithAutoScroll(AutoScrollPolicy.None) .WithContent(_toolRowsNode); _toolScrollNode.Fill(); - layout = layout.WithChild(_toolScrollNode); + // ScrollableContainerNode.Render ignores bounds.Y and writes relative to the + // context's (0,0). Hosting it inside a borderless PanelNode ensures the + // PanelNode calls context.CreateSubContext(bounds) first, so the scroll + // container receives a context already offset to its actual screen position. + layout = layout.WithChild( + new PanelNode() + .WithBorder(BorderStyle.None) + .WithContent(_toolScrollNode) + .Fill()); return layout; } @@ -226,7 +234,15 @@ private void EnsureToolCursorVisible() { if (_toolScrollNode is null || _gridCursor < FirstToolRow) return; var toolIdx = _gridCursor - FirstToolRow; - if (_toolScrollNode.MaxScroll == 0) return; + if (_toolScrollNode.MaxScroll == 0) + { + // All tools fit in the viewport. Reset any stale offset left over from + // a prior larger scroll position (e.g. after a terminal resize or after + // the audience changes to a smaller visible tool set). + if (_toolScrollNode.ScrollOffset != 0) + _toolScrollNode.ScrollTo(0); + return; + } var viewportH = _toolScrollNode.ContentHeight - _toolScrollNode.MaxScroll; if (viewportH <= 0) return; if (toolIdx < _toolScrollNode.ScrollOffset) @@ -249,7 +265,7 @@ private LayoutNode BuildFooter() if (_confirmingSave) { return new TextNode("Save changes? [Y] Yes [N] No [Esc] Cancel") - .WithForeground(Color.Yellow).Bold(); + .WithForeground(Color.Yellow).Bold().NoWrap(); } var hints = ViewModel.CurrentState.Value switch @@ -266,28 +282,14 @@ private LayoutNode BuildFooter() var hasStatus = !string.IsNullOrEmpty(statusText); if (ViewModel.HasSaveError) - { - return Layouts.Horizontal() - .WithChild(new TextNode(hints).WithForeground(Color.BrightBlack)) - .WithChild(new TextNode($" {statusText}").WithForeground(Color.Red)); - } - + return BuildToolGridFooterWithStatus(hints, $" {statusText}", Color.Red); if (ViewModel.HasUnsavedChanges) - { - return Layouts.Horizontal() - .WithChild(new TextNode(hints).WithForeground(Color.BrightBlack)) - .WithChild(new TextNode(" *unsaved*").WithForeground(Color.Yellow)); - } - + return BuildToolGridFooterWithStatus(hints, " *unsaved*", Color.Yellow); if (hasStatus) - { - return Layouts.Horizontal() - .WithChild(new TextNode(hints).WithForeground(Color.BrightBlack)) - .WithChild(new TextNode($" {statusText}").WithForeground(Color.Green)); - } + return BuildToolGridFooterWithStatus(hints, $" {statusText}", Color.Green); } - return new TextNode(hints).WithForeground(Color.BrightBlack); + return new TextNode(hints).WithForeground(Color.BrightBlack).NoWrap(); }); ViewModel.StateVersion @@ -297,6 +299,13 @@ private LayoutNode BuildFooter() return _footerNode; } + private static LayoutNode BuildToolGridFooterWithStatus(string hints, string status, Color color) + { + return Layouts.Horizontal() + .WithChild(new TextNode(hints).WithForeground(Color.BrightBlack).NoWrap()) + .WithChild(new TextNode(status).WithForeground(color).WidthAuto()); + } + private void HandleKeyPress(KeyPressed key) { var keyInfo = key.KeyInfo; diff --git a/tests/smoke/screenshots/mcp-permissions-server-list.approved.png b/tests/smoke/screenshots/mcp-permissions-server-list.approved.png new file mode 100644 index 000000000..94100e399 Binary files /dev/null and b/tests/smoke/screenshots/mcp-permissions-server-list.approved.png differ diff --git a/tests/smoke/screenshots/mcp-permissions-tool-grid.approved.png b/tests/smoke/screenshots/mcp-permissions-tool-grid.approved.png new file mode 100644 index 000000000..d8b8659d8 Binary files /dev/null and b/tests/smoke/screenshots/mcp-permissions-tool-grid.approved.png differ diff --git a/tests/smoke/tapes/screenshots/mcp-permissions.tape b/tests/smoke/tapes/screenshots/mcp-permissions.tape new file mode 100644 index 000000000..eb0dd80c2 --- /dev/null +++ b/tests/smoke/tapes/screenshots/mcp-permissions.tape @@ -0,0 +1,88 @@ +# mcp-permissions.tape (screenshot) — capture `netclaw mcp permissions` with +# the smoke MCP server (smoke-math) connected and its tools indexed. +# +# Capture-only tape: NO post-tape assertion. The screenshots mode in +# run-smoke.sh compares each emitted PNG byte-for-byte against the committed +# baseline at tests/smoke/screenshots/.approved.png. +# +# Frames captured: +# mcp-permissions-server-list — ServerList state: smoke-math appears as +# "Connected, 3 tools" below the header. +# mcp-permissions-tool-grid — ToolGrid state: header rows (Audience, +# Server enabled, Server default) and all +# three tool rows (add, echo, record-tasks) +# simultaneously visible. This is the direct +# regression check for issue #1424 — the +# scroll container must not overwrite the header. +# +# Why these frames matter: the Loading/no-daemon frame only confirmed the +# header layout compiles. These frames confirm the layout holds under real +# data: a connected server, its tool rows rendered at the correct vertical +# offset, and all metadata rows above them untouched. +# +# Prepended preamble: tapes/screenshot-preamble.tape (determinism-pinned). +# __NETCLAW_SMOKE_MCP_SERVER__ is substituted by run-native-tape.sh. + +Output "/tmp/tape-shot-mcp-permissions.gif" + +# ─── Setup: seed provider + register the smoke MCP server ──────────── +# The smoke Ollama instance is already running at 127.0.0.1:11434 in +# screenshots mode. We seed a minimal provider so the daemon starts cleanly. +Hide +Type "netclaw provider add smoke-llm ollama --endpoint http://127.0.0.1:11434" +Enter +Wait+Screen@10s /TAPE\$/ + +Type "netclaw model set main smoke-llm qwen2:0.5b" +Enter +Wait+Screen@10s /TAPE\$/ + +# Register the deterministic smoke MCP server (add, echo, record-tasks). +# --grant-all means every tool is auto-approved for all audiences. +Type "netclaw mcp add --transport stdio --grant-all smoke-math -- __NETCLAW_SMOKE_MCP_SERVER__" +Enter +Wait+Screen@10s /TAPE\$/ + +# Start the daemon (detaches immediately; MCP handshake follows async). +Type "netclaw daemon start" +Enter +Wait+Screen@20s /TAPE\$/ + +# Allow time for the daemon to spawn the stdio MCP process and index tools. +Sleep 5s +Show + +# ─── Launch the TUI ────────────────────────────────────────────────── +Type "netclaw mcp permissions" +Enter + +# ─── Frame 1: ServerList ───────────────────────────────────────────── +# smoke-math should appear as "Connected, 3 tools". +Wait+Screen@15s /smoke-math/ +Sleep 1s +Screenshot "/tmp/shot-mcp-permissions-server-list.png" +Sleep 1s + +# ─── Navigate into the ToolGrid ────────────────────────────────────── +Enter + +# ─── Frame 2: ToolGrid ─────────────────────────────────────────────── +# All header rows (Server, Audience, Server enabled, Server default) plus +# all tool rows (add, echo, record-tasks) must be visible simultaneously. +# If the #1424 regression reappears, tool rows will overwrite the header. +Wait+Screen@10s /record-tasks/ +Sleep 1s +Screenshot "/tmp/shot-mcp-permissions-tool-grid.png" +Sleep 1s + +# ─── Exit TUI ──────────────────────────────────────────────────────── +Ctrl+Q +Wait+Screen@10s /TAPE\$/ + +# Stop the daemon so the tape-level cleanup does not race the next tape. +Type "netclaw daemon stop" +Enter +Wait+Screen@10s /TAPE\$/ + +Type "exit" +Enter