From a0ae9ba270a59af156ea7d1fe696f76615d221e2 Mon Sep 17 00:00:00 2001 From: Prateek Singh Date: Thu, 6 Aug 2026 18:31:58 -0400 Subject: [PATCH 1/3] fix(mcp): stop search_project leaking $HOME and credentials; clamp goto_line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four defects reported from Windows testing, plus everything an adversarial review found in the fixes themselves. NP-01 goto_line past EOF lied. Scintilla rejects an out-of-range position and leaves the cursor at the TOP, while the response still read {"line":99999, "ok":true}. Since insert_text defaults to the cursor, an assistant aiming at the end of a file wrote at the beginning, behind an approval card that looked correct. Editor::gotoLine now clamps and RETURNS where it landed; the verb reports line/requested_line/clamped. NP-02 search_project walked the user's whole profile. FileExplorer's ctor set m_rootPath = QDir::homePath() as a display placeholder, and four separate guards read that as "the workspace" — including the AI CSV sandbox, a security guard that had therefore never once closed, and the Coding Mode prompt, which was dead code. Display root and workspace root are now different questions: workspaceRoot() stays empty until the user opens a folder. MainWindow grew four accessors in place of one, because "what folder is the user working on" has four different right answers: workspaceFolder() (scoping, empty means refuse), firstOpenFileDir(), suggestedDialogFolder() (may fall back broadly — the user sees and edits it), aiWorkspaceRoot() (sticky per session). The first fix collapsed them again as "folder, else current file's directory", which resolves to $HOME the moment one file in $HOME is open, and made the AI root flap on every Ctrl+Tab — swapping the chat history and cancelling pending write approvals. Chat history migrates from the old sha1($HOME) key by COPY. NP-03 the sidecar hung on every bad argument. --version printed nothing and blocked on stdin; --sokcet silently started the MOCK against fabricated tabs; a mistyped subcommand fell through the same way. All now exit 2 loudly. NP-04 the Windows PE carried FileVersion 0,1,0,0 for 124 releases. The .rc is generated from CMake now, with a CI step reading VersionInfo back. UNVERIFIED on any platform — the gate has never run. search_project also honours the credential deny-list. That list lived in TWO places and had drifted in BOTH directions: ai_tools had *.tfvars/*.tfstate/ .pypirc/dotenv/backslash variants, git_hunk_apply had *.jks and an unanchored id_rsa, neither was a superset, and search_project consulted neither. So read_file refused ~/.ssh/id_rsa while a one-word search returned its lines. src/path_denylist.h is now the only list — the union — with three callers. insert_text with col but no line silently dropped col and wrote at the cursor instead. Silently relocating a write is the one thing an approval gate cannot protect against, so it is an error now. Tests: 76/76 C++, 91 Rust. Every new test red-state verified by restoring the pre-fix code rather than negating a condition — a hand-written sabotage of search_project still returned the right answer and made a sound test look weak. Signatures matched the original bug reports: {"line":99999, "clamped":false}, workspace_searched:true with no workspace, and "leaked contents of keystore.jks" — the extension only the git_hunk_apply copy of the deny-list had. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01W6BGY2dFUSujZeS3vTDikq --- .github/workflows/build.yml | 27 +++ CMakeLists.txt | 46 +++- notepatra-mcp/mcpb/build-mcpb.py | 16 ++ notepatra-mcp/src/main.rs | 69 +++++- notepatra-mcp/src/server.rs | 8 +- notepatra-mcp/src/tools.rs | 61 +++-- notepatra-mcp/src/transport/mock.rs | 17 +- notepatra-mcp/src/transport/mod.rs | 5 +- notepatra-mcp/src/transport/socket.rs | 10 +- notepatra-mcp/tests/cli.rs | 93 ++++++++ notepatra-mcp/tests/protocol.rs | 89 +++++++ resources/{notepatra.rc => notepatra.rc.in} | 17 +- src/ai_tools.cpp | 67 +----- src/aipanel.cpp | 35 ++- src/editor.cpp | 18 +- src/editor.h | 3 +- src/fileexplorer.cpp | 34 ++- src/fileexplorer.h | 21 +- src/git_hunk_apply.cpp | 26 +- src/mainwindow.cpp | 123 +++++++--- src/mainwindow.h | 40 ++++ src/mcp_bridge.cpp | 94 +++++++- src/mcp_bridge.h | 6 +- src/path_denylist.cpp | 73 ++++++ src/path_denylist.h | 25 ++ test_ai_fullscreen_exit.cpp | 87 +++++++ test_mcp_bridge.cpp | 252 +++++++++++++++++++- test_workspace_root.cpp | 148 ++++++++++++ 28 files changed, 1344 insertions(+), 166 deletions(-) create mode 100644 notepatra-mcp/tests/cli.rs rename resources/{notepatra.rc => notepatra.rc.in} (64%) create mode 100644 src/path_denylist.cpp create mode 100644 src/path_denylist.h create mode 100644 test_workspace_root.cpp diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 481959f..7ffae77 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -1212,6 +1212,33 @@ jobs: echo no tracker file — no transport suite reached its first test ) + # Read the PE version resource back off the built .exe. + # + # resources/notepatra.rc carried a hardcoded FILEVERSION 0,1,0,0 for 124 + # releases, so winget / SCCM / Intune and File Properties reported 0.1.0 + # on every machine ever. It is now generated from project(Notepatra + # VERSION ...) — and this gate is what keeps it that way, because nothing + # else on any platform can observe an embedded Windows resource. + - name: Assert PE FileVersion matches the project version + shell: pwsh + run: | + $cmakeVer = (Select-String -Path CMakeLists.txt ` + -Pattern 'project\(Notepatra VERSION ([0-9]+\.[0-9]+\.[0-9]+)' ` + | Select-Object -First 1).Matches.Groups[1].Value + if (-not $cmakeVer) { Write-Host "::error::could not read version from CMakeLists.txt"; exit 1 } + $expected = "$cmakeVer.0" + $exe = Get-ChildItem -Path build -Filter notepatra.exe -Recurse -ErrorAction SilentlyContinue | Select-Object -First 1 + if (-not $exe) { Write-Host "::error::notepatra.exe not found under build/"; exit 1 } + $actual = (Get-Item $exe.FullName).VersionInfo.FileVersion + Write-Host "CMakeLists version : $cmakeVer" + Write-Host "expected FileVersion: $expected" + Write-Host "actual FileVersion: $actual" + if ($actual -ne $expected) { + Write-Host "::error::PE FileVersion '$actual' != expected '$expected' — the .rc template did not substitute" + exit 1 + } + Write-Host "OK: PE version resource matches the project version" + # Separate step because the assertion needs pwsh string matching, and cmd # has no clean equivalent. `serve` on a std-only build exits 2 after # printing this, so we match on the text rather than the exit code. diff --git a/CMakeLists.txt b/CMakeLists.txt index 267e10c..c9190cd 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -298,9 +298,29 @@ set(HEADERS # ── Platform-specific resources ── if(WIN32) - # Windows: embed icon via .rc resource file - if(EXISTS "${CMAKE_SOURCE_DIR}/resources/notepatra.rc") - list(APPEND SOURCES resources/notepatra.rc) + # Windows: embed the icon AND the version resource via a GENERATED .rc. + # + # The .rc used to be a static file with FILEVERSION hardcoded to 0,1,0,0, + # so every Windows build reported 0.1.0 to winget / SCCM / Intune and to + # File Properties > Details, regardless of the real version. Generating it + # from project(Notepatra VERSION ...) makes that impossible to drift: there + # is exactly one place the version is written. + if(EXISTS "${CMAKE_SOURCE_DIR}/resources/notepatra.rc.in") + # VERSIONINFO needs a 4-part comma form (a,b,c,d); PROJECT_VERSION is + # 3-part dotted, so pad the build field with 0. + set(NOTEPATRA_RC_VERSION_COMMA + "${PROJECT_VERSION_MAJOR},${PROJECT_VERSION_MINOR},${PROJECT_VERSION_PATCH},0") + set(NOTEPATRA_RC_VERSION_DOT + "${PROJECT_VERSION_MAJOR}.${PROJECT_VERSION_MINOR}.${PROJECT_VERSION_PATCH}.0") + # The generated .rc lives in the build tree, where a bare + # "notepatra.ico" would not resolve — point at the source copy. RC + # wants forward or escaped slashes, so hand it a forward-slash path. + set(NOTEPATRA_ICON_PATH "${CMAKE_SOURCE_DIR}/resources/notepatra.ico") + configure_file( + "${CMAKE_SOURCE_DIR}/resources/notepatra.rc.in" + "${CMAKE_BINARY_DIR}/notepatra.rc" + @ONLY) + list(APPEND SOURCES "${CMAKE_BINARY_DIR}/notepatra.rc") endif() endif() @@ -586,6 +606,7 @@ if(NOTEPATRA_BUILD_TESTS AND EXISTS "${CMAKE_SOURCE_DIR}/test_indent_guide_theme ${CMAKE_SOURCE_DIR}/src/rustbridge.cpp ${CMAKE_SOURCE_DIR}/src/gitgutter.cpp ${CMAKE_SOURCE_DIR}/src/git_hunk_apply.cpp + ${CMAKE_SOURCE_DIR}/src/path_denylist.cpp ${CMAKE_SOURCE_DIR}/src/gutter_hunk_popup.cpp ${CMAKE_SOURCE_DIR}/src/diff_view.cpp) add_dependencies(test_indent_guide_theme rust_core) @@ -1096,6 +1117,7 @@ if(NOTEPATRA_BUILD_TESTS AND EXISTS "${CMAKE_SOURCE_DIR}/test_ai_tools.cpp") add_executable(test_ai_tools test_ai_tools.cpp src/ai_tools.cpp + src/path_denylist.cpp src/git_tools.cpp src/csvanalyst.cpp src/dbconnections.cpp @@ -1118,6 +1140,7 @@ if(NOTEPATRA_BUILD_TESTS AND EXISTS "${CMAKE_SOURCE_DIR}/test_git_tools_e2e.cpp" add_executable(test_git_tools_e2e test_git_tools_e2e.cpp src/ai_tools.cpp + src/path_denylist.cpp src/git_tools.cpp src/csvanalyst.cpp src/dbconnections.cpp @@ -1140,6 +1163,7 @@ if(NOTEPATRA_BUILD_TESTS AND EXISTS "${CMAKE_SOURCE_DIR}/test_ai_dataanalyst.cpp test_ai_dataanalyst.cpp src/ai_systemprompt.cpp src/ai_tools.cpp + src/path_denylist.cpp src/git_tools.cpp src/chartrender.cpp src/chart_modal.cpp @@ -1839,6 +1863,19 @@ if(NOTEPATRA_BUILD_TESTS AND EXISTS "${CMAKE_SOURCE_DIR}/test_single_instance.cp message(STATUS "Regression test enabled — target: test_single_instance") endif() +# Workspace-root scoping. FileExplorer::workspaceRoot() must stay empty until +# the user opens a folder — conflating it with the tree's display root (which +# defaults to $HOME) made search_project walk the whole user profile and +# disarmed the AI CSV sandbox guard. +if(NOTEPATRA_BUILD_TESTS AND EXISTS "${CMAKE_SOURCE_DIR}/test_workspace_root.cpp") + add_executable(test_workspace_root test_workspace_root.cpp src/fileexplorer.cpp) + target_include_directories(test_workspace_root PRIVATE ${CMAKE_SOURCE_DIR}/src) + target_link_libraries(test_workspace_root PRIVATE Qt5::Widgets Qt5::Test) + notepatra_add_qt_test(test_workspace_root) + set_tests_properties(test_workspace_root PROPERTIES LABELS "smoke;privacy;mcp") + message(STATUS "Regression test enabled — target: test_workspace_root") +endif() + # D7 (win-open-ghost) — CLI arg parse contract (the Windows wide-argv fix # feeds QCoreApplication::arguments() into this parser). if(NOTEPATRA_BUILD_TESTS AND EXISTS "${CMAKE_SOURCE_DIR}/test_cli_args.cpp") @@ -2610,6 +2647,7 @@ if(NOTEPATRA_BUILD_TESTS AND EXISTS "${CMAKE_SOURCE_DIR}/test_mcp_bridge.cpp") add_executable(test_mcp_bridge test_mcp_bridge.cpp src/mcp_bridge.cpp + src/path_denylist.cpp src/singleinstance.cpp # v0.1.118 — list_notes/read_note go through the real NotesStorage # layer (QtCore-only; notes_template.cpp resolves shellHtml). @@ -2884,6 +2922,7 @@ if(NOTEPATRA_BUILD_TESTS AND EXISTS "${CMAKE_SOURCE_DIR}/test_options_actually_w src/editor_symbols.cpp # Editor::applySymbolSettings() links against it src/gitgutter.cpp src/git_hunk_apply.cpp + src/path_denylist.cpp src/gutter_hunk_popup.cpp src/diff_view.cpp src/lexerutils.cpp @@ -3034,6 +3073,7 @@ if(NOTEPATRA_BUILD_TESTS AND EXISTS "${CMAKE_SOURCE_DIR}/test_vega_chart.cpp") add_executable(test_vega_chart test_vega_chart.cpp src/ai_tools.cpp + src/path_denylist.cpp src/git_tools.cpp src/csvanalyst.cpp src/dbconnections.cpp diff --git a/notepatra-mcp/mcpb/build-mcpb.py b/notepatra-mcp/mcpb/build-mcpb.py index ac8557d..3dc1b64 100755 --- a/notepatra-mcp/mcpb/build-mcpb.py +++ b/notepatra-mcp/mcpb/build-mcpb.py @@ -104,6 +104,22 @@ def main(): dcmd = cfg.get("platform_overrides", {}).get("darwin", {}).get("command", "") if not dcmd.endswith("server/darwin/notepatra-mcp"): fail(f"manifest darwin override '{dcmd}' does not point at server/darwin/notepatra-mcp") + # win32 was the ONLY platform whose override was never checked — the same + # blind spot that let the Windows build go unverified everywhere else. + wcmd = cfg.get("platform_overrides", {}).get("win32", {}).get("command", "") + if not wcmd.endswith("server/win32-x64/notepatra-mcp.exe"): + fail(f"manifest win32 override '{wcmd}' does not point at server/win32-x64/notepatra-mcp.exe") + # Every platform must end up launching with --socket. Overrides that specify + # only `command` inherit the base args, so the effective args are the base + # ones unless a platform sets its own; assert on the resolved value rather + # than trusting that inheritance, because losing this flag silently swaps the + # real editor for the in-memory MOCK and every tool returns fabricated data. + base_args = cfg.get("args", []) + for plat, ov in list(cfg.get("platform_overrides", {}).items()) + [("", {})]: + eff = ov.get("args", base_args) + if "--socket" not in eff: + fail(f"platform '{plat}' resolves to args {eff} — missing --socket, " + f"so it would run the in-memory mock instead of the editor") if a.require_all: for plat in ALL_PLATFORMS: if not any(n.startswith(f"server/{plat}/") for n in names): diff --git a/notepatra-mcp/src/main.rs b/notepatra-mcp/src/main.rs index 2a224e0..fef77ed 100644 --- a/notepatra-mcp/src/main.rs +++ b/notepatra-mcp/src/main.rs @@ -2,15 +2,80 @@ use notepatra_mcp::server::Server; use notepatra_mcp::transport::{mock::MockEditor, socket::SocketEditor}; +const USAGE: &str = "\ +notepatra-mcp — Model Context Protocol server for the Notepatra editor + +USAGE: + notepatra-mcp [--socket] + notepatra-mcp (requires the `remote` feature) + +OPTIONS: + --socket Drive the RUNNING editor over its local MCP bridge. Without + this the server answers from an in-memory MOCK whose data is + fabricated — useful for exercising the protocol, never for + real work. + -h, --help Print this help and exit. + -V, --version Print the version and exit. + +This is a stdio server: with no flags it speaks JSON-RPC on stdin/stdout and +will appear to hang if you run it by hand. That is expected — it is meant to be +launched by an MCP client, not a terminal. + +Docs: https://notepatra.org/mcp.html"; + fn main() -> std::io::Result<()> { // Phase 3a subcommands (serve/pair/connect) are dispatched FIRST and only // when they are the first argument, so every existing invocation — no args, - // `--socket`, anything else — reaches the unchanged stdio path below, - // byte-for-byte identical to HEAD. + // `--socket`, anything else — reaches the unchanged stdio path below. if let Some(mode @ ("serve" | "pair" | "connect")) = std::env::args().nth(1).as_deref() { return run_remote_mode(mode); } + // A MISTYPED subcommand must fail loudly too. + // + // Rejecting only bad flags left half the hang in place: USAGE advertises + // bare-word subcommands, so `notepatra-mcp sevre` fell straight through to + // the stdio loop and blocked on stdin forever — the same "looks crashed" + // symptom, reached by the other spelling mistake. + if let Some(first) = std::env::args().nth(1) { + if !first.starts_with('-') && !matches!(first.as_str(), "serve" | "pair" | "connect") { + eprintln!("notepatra-mcp: unknown subcommand '{first}'"); + eprintln!("Expected one of: serve, pair, connect"); + eprintln!("Try 'notepatra-mcp --help' for usage."); + std::process::exit(2); + } + } + + // Discovery flags, and rejection of anything else that LOOKS like a flag. + // + // Before v0.1.125 none of this existed: every unrecognised argument fell + // through to the stdio loop below, which blocks reading stdin. So + // `--version` printed nothing and hung, and a typo like `--sokcet` silently + // started a MOCK server instead of failing. Both read as a crashed program. + // An unknown flag must be a loud error, never a hang. + for arg in std::env::args().skip(1) { + match arg.as_str() { + "-h" | "--help" => { + println!("{USAGE}"); + return Ok(()); + } + "-V" | "--version" => { + println!("notepatra-mcp {}", env!("CARGO_PKG_VERSION")); + return Ok(()); + } + "--socket" => {} + // Bare words are left alone: a client may pass a path or an + // extra positional we do not own. Only flag-shaped typos are + // fatal, because those are ours to get wrong. + other if other.starts_with('-') => { + eprintln!("notepatra-mcp: unrecognised option '{other}'"); + eprintln!("Try 'notepatra-mcp --help' for usage."); + std::process::exit(2); + } + _ => {} + } + } + // `--socket` targets the running editor over its dedicated MCP bridge // socket; default is the in-memory mock so any MCP client can exercise // the protocol without a running editor. diff --git a/notepatra-mcp/src/server.rs b/notepatra-mcp/src/server.rs index 382740c..a0e0e45 100644 --- a/notepatra-mcp/src/server.rs +++ b/notepatra-mcp/src/server.rs @@ -31,8 +31,12 @@ const NOTE_URI_PREFIX: &str = "notepatra://note/"; pub struct Server { transport: T, /// serverInfo.version, resolved once at startup: NOTEPATRA_MCP_VERSION - /// (set by the editor when it spawns the sidecar) wins over this crate's - /// own version. + /// wins over this crate's own version if it is set. + /// + /// NOTE: nothing in the repo currently SETS that variable — the editor does + /// not spawn the sidecar (MCP clients launch it), so in every shipped + /// configuration this is CARGO_PKG_VERSION. The override is kept as an + /// escape hatch for a packager who ships a differently-versioned binary. version: String, } diff --git a/notepatra-mcp/src/tools.rs b/notepatra-mcp/src/tools.rs index 1dc3506..1f6efe8 100644 --- a/notepatra-mcp/src/tools.rs +++ b/notepatra-mcp/src/tools.rs @@ -210,7 +210,7 @@ pub fn definitions() -> Value { }, { "name": "goto_line", - "description": "Move the editor cursor to a 1-based line number in a tab (defaults to the active tab). Use to point the user at a specific location, e.g. after finding a match.", + "description": "Move the editor cursor to a 1-based line number in a tab (defaults to the active tab). Use to point the user at a specific location, e.g. after finding a match. Lines past end-of-file are CLAMPED to the last line: the response's `line` is where the cursor actually landed and `clamped` is true, so check it before any cursor-relative write.", "inputSchema": { "type": "object", "properties": { @@ -779,23 +779,53 @@ fn optional_index(args: &Map, key: &str) -> Result, } } +/// The ONE message every 1-based-integer rejection uses. +/// +/// These helpers used to delegate to `optional_index`/`required_index`, whose +/// `as_u64()` rejected negatives with "must be a non-negative integer" before +/// the `>= 1` check was ever reached. So `line: 0` and `line: -5` — the same +/// mistake — came back with two different and mutually contradictory rules, one +/// of which explicitly permits 0. Validating here keeps it to one sentence. +fn one_based_error(key: &str) -> CallOutcome { + CallOutcome::InvalidParams(format!("{key} must be an integer >= 1")) +} + +/// The C++ bridge reads these through QJsonValue::toInt(0), which yields 0 — +/// and therefore the misleading "must be >= 1" — for anything above i32::MAX. +/// Reject out of range here instead, with a message that names the real limit. +const MAX_ONE_BASED: u64 = i32::MAX as u64; + +fn check_one_based(key: &str, n: u64) -> Result { + if n == 0 { + return Err(one_based_error(key)); + } + if n > MAX_ONE_BASED { + return Err(CallOutcome::InvalidParams(format!( + "{key} must be an integer >= 1 and <= {MAX_ONE_BASED}" + ))); + } + Ok(n as usize) +} + /// Optional 1-based integer (line/column numbers). fn optional_one_based(args: &Map, key: &str) -> Result, CallOutcome> { - match optional_index(args, key)? { - Some(0) => Err(CallOutcome::InvalidParams(format!( - "{key} must be an integer >= 1" - ))), - v => Ok(v), + match args.get(key) { + None | Some(Value::Null) => Ok(None), + Some(v) => match v.as_u64() { + None => Err(one_based_error(key)), + Some(n) => check_one_based(key, n).map(Some), + }, } } /// Required 1-based integer (line/column numbers). fn required_one_based(args: &Map, key: &str) -> Result { - match required_index(args, key)? { - 0 => Err(CallOutcome::InvalidParams(format!( - "{key} must be an integer >= 1" - ))), - n => Ok(n), + match args.get(key) { + None => Err(CallOutcome::InvalidParams(format!("{key} is required"))), + Some(v) => match v.as_u64() { + None => Err(one_based_error(key)), + Some(n) => check_one_based(key, n), + }, } } @@ -965,9 +995,12 @@ fn goto_line(transport: &mut dyn EditorTransport, args: &Map) -> if let Err(e) = reject_extras(args, &["line", "tab_index"]) { return e; } - let line = match required_index(args, "line") { - Ok(n) if n >= 1 => n, - Ok(_) => return CallOutcome::InvalidParams("line must be an integer >= 1".into()), + // required_one_based, NOT a hand-rolled required_index + `>= 1` check: + // select_range and insert_text already route every 1-based argument through + // the shared helper, and goto_line being the one exception is what produced + // two contradictory rules for the same mistake. + let line = match required_one_based(args, "line") { + Ok(n) => n, Err(e) => return e, }; let tab_index = match optional_index(args, "tab_index") { diff --git a/notepatra-mcp/src/transport/mock.rs b/notepatra-mcp/src/transport/mock.rs index 44809f8..6d9fcb2 100644 --- a/notepatra-mcp/src/transport/mock.rs +++ b/notepatra-mcp/src/transport/mock.rs @@ -485,8 +485,23 @@ impl EditorTransport for MockEditor { if line == 0 { return Err(TransportError("line numbers are 1-based".into())); } + // Clamp exactly as the C++ bridge does, and report the line LANDED on. + // + // This mock is the default transport of the shipped binary (no + // `--socket`) and every protocol test runs through it — so while it + // echoed the requested line it reproduced the NP-01 false report + // verbatim (`ok:true, line:99999` on a 4-line tab) and no Rust test + // could ever have observed the fixed response shape. + let total = self.tabs[i].content.lines().count().max(1); + let landed = line.min(total); self.selection.0 = i; - Ok(json!({ "ok": true, "tab_index": i, "line": line })) + Ok(json!({ + "ok": true, + "tab_index": i, + "line": landed, + "requested_line": line, + "clamped": landed != line, + })) } fn select_range( diff --git a/notepatra-mcp/src/transport/mod.rs b/notepatra-mcp/src/transport/mod.rs index fe44c6a..f4cc6c7 100644 --- a/notepatra-mcp/src/transport/mod.rs +++ b/notepatra-mcp/src/transport/mod.rs @@ -109,7 +109,10 @@ impl std::error::Error for TransportError {} /// * `list_recent_files` → `{files:[str]}` /// * `find_in_tab` → `{matches:[{line,text}],truncated}` /// * `new_tab` → `{tab_index}` -/// * `goto_line` → `{ok,tab_index,line}` +/// * `goto_line` → `{ok,tab_index,line,requested_line,clamped}` — `line` is +/// where the cursor LANDED, not what was asked for: a line past end-of-file +/// clamps to the last line and sets `clamped:true`. Check it before issuing a +/// cursor-relative write. /// * `set_language` → `{ok,tab_index,language}` /// * `compare_tabs` → `{opened}` /// * `format_text` → `{text}` diff --git a/notepatra-mcp/src/transport/socket.rs b/notepatra-mcp/src/transport/socket.rs index a6a704b..34b1cb3 100644 --- a/notepatra-mcp/src/transport/socket.rs +++ b/notepatra-mcp/src/transport/socket.rs @@ -141,9 +141,13 @@ impl SocketEditor { )) } - /// Targets an explicit socket path, BYPASSING discovery (`--socket-path` - /// and tests with a fake bridge). Exactly one candidate, so an explicit - /// path can never silently fall through to some other editor. + /// Targets an explicit socket path, BYPASSING discovery. Exactly one + /// candidate, so an explicit path can never silently fall through to some + /// other editor. + /// + /// Used by the tests' fake bridge. There is no `--socket-path` flag — an + /// earlier version of this comment named one, and main.rs now rejects + /// unknown flags outright, so passing it would exit 2. pub fn with_socket_path(path: impl Into) -> Self { Self::with_candidates(vec![path.into()]) } diff --git a/notepatra-mcp/tests/cli.rs b/notepatra-mcp/tests/cli.rs new file mode 100644 index 0000000..a1e482c --- /dev/null +++ b/notepatra-mcp/tests/cli.rs @@ -0,0 +1,93 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +// +// Command-line surface of the `notepatra-mcp` binary. +// +// This is a stdio server: with no arguments it blocks reading stdin, which is +// correct when an MCP client launched it and looks exactly like a crash when a +// human ran it in a terminal. Before v0.1.125 EVERY unrecognised argument fell +// into that loop — `--version` printed nothing and hung, `--sokcet` silently +// started a MOCK server against fabricated data, and a mistyped subcommand like +// `sevre` hung too. The rule these tests pin: an argument we do not understand +// is a loud exit, never a hang and never a silent fallback. +// +// Every child gets a null stdin, so a regression cannot wedge the test suite — +// it would hit EOF and exit 0, which is still a failure against `code == 2`. + +use std::process::{Command, Stdio}; + +fn run(args: &[&str]) -> (i32, String, String) { + let out = Command::new(env!("CARGO_BIN_EXE_notepatra-mcp")) + .args(args) + .stdin(Stdio::null()) + .output() + .expect("failed to spawn notepatra-mcp"); + ( + out.status.code().unwrap_or(-1), + String::from_utf8_lossy(&out.stdout).into_owned(), + String::from_utf8_lossy(&out.stderr).into_owned(), + ) +} + +#[test] +fn help_and_version_print_and_exit_zero() { + for flag in ["-h", "--help"] { + let (code, stdout, _) = run(&[flag]); + assert_eq!(code, 0, "{flag} should exit 0"); + assert!( + stdout.contains("USAGE"), + "{flag} printed no usage block: {stdout}" + ); + } + for flag in ["-V", "--version"] { + let (code, stdout, _) = run(&[flag]); + assert_eq!(code, 0, "{flag} should exit 0"); + assert!( + stdout.contains(env!("CARGO_PKG_VERSION")), + "{flag} printed no version: {stdout}" + ); + } +} + +#[test] +fn a_mistyped_subcommand_exits_two_instead_of_hanging() { + // USAGE advertises bare-word subcommands, so this is a realistic typo — + // and it is the half that survived the first fix, which rejected only + // flag-shaped arguments. + let (code, _, stderr) = run(&["sevre"]); + assert_eq!(code, 2, "unknown subcommand must exit 2, got {code}"); + assert!( + stderr.contains("unknown subcommand") && stderr.contains("sevre"), + "the error must name what was typed: {stderr}" + ); + assert!( + stderr.contains("serve"), + "the error must list the real subcommands: {stderr}" + ); +} + +#[test] +fn a_mistyped_flag_exits_two_rather_than_starting_a_mock() { + // The dangerous half: `--sokcet` used to start the in-memory MOCK, so the + // client got fabricated tabs that looked like the user's real editor. + let (code, _, stderr) = run(&["--sokcet"]); + assert_eq!(code, 2, "unknown flag must exit 2, got {code}"); + assert!( + stderr.contains("--sokcet"), + "the error must name the offending flag: {stderr}" + ); +} + +#[test] +fn the_real_subcommands_are_not_swallowed_by_the_typo_guard() { + // Vacuity guard for the two tests above: if the guard rejected everything, + // they would pass while the binary was broken. Built without the `remote` + // feature these exit 2 as well, but with a DIFFERENT message — so assert on + // the message, not the code. + for mode in ["serve", "pair", "connect"] { + let (_, _, stderr) = run(&[mode]); + assert!( + !stderr.contains("unknown subcommand"), + "{mode} is a real subcommand and must not hit the typo guard: {stderr}" + ); + } +} diff --git a/notepatra-mcp/tests/protocol.rs b/notepatra-mcp/tests/protocol.rs index 033ec93..7e13254 100644 --- a/notepatra-mcp/tests/protocol.rs +++ b/notepatra-mcp/tests/protocol.rs @@ -454,6 +454,95 @@ fn wave2_malformed_arguments_are_invalid_params_errors() { } } +/// goto_line must report where the cursor LANDED, never echo the request. +/// +/// The mock is the default transport of the shipped binary, and every test in +/// this file runs through it — so while it echoed the requested line, the Rust +/// suite could pass with the NP-01 false report fully intact. It did, for the +/// whole of the fix that was supposed to remove it. +#[test] +fn goto_line_past_eof_clamps_and_reports_where_it_landed() { + // Mock tab 0 is a 3-line Rust file; derive the count rather than hardcode. + let read = run_lines(&[call_line(1, "read_tab", json!({ "tab_index": 0 }))]); + let total = text_of(&read[0]).lines().count(); + assert!(total > 0, "vacuity guard: mock tab 0 must have content"); + + let responses = run_lines(&[ + call_line(2, "goto_line", json!({ "line": 99999, "tab_index": 0 })), + call_line(3, "goto_line", json!({ "line": 1, "tab_index": 0 })), + ]); + + let past: Value = serde_json::from_str(text_of(&responses[0])).unwrap(); + assert_eq!( + past["line"].as_u64(), + Some(total as u64), + "expected a clamp to the last line, got {past}" + ); + assert_ne!( + past["line"].as_u64(), + Some(99999), + "goto_line echoed the requested line instead of the real one: {past}" + ); + assert_eq!(past["requested_line"].as_u64(), Some(99999)); + assert_eq!(past["clamped"].as_bool(), Some(true)); + + // An in-range jump must not be flagged as clamped. + let ok: Value = serde_json::from_str(text_of(&responses[1])).unwrap(); + assert_eq!(ok["line"].as_u64(), Some(1)); + assert_eq!(ok["clamped"].as_bool(), Some(false)); +} + +/// A 1-based argument must be rejected with ONE rule, whatever the bad value. +/// +/// `required_one_based` used to delegate to `required_index`, whose `as_u64()` +/// rejected negatives before the `>= 1` check ran. So the same mistake produced +/// two contradictory sentences — `line: 0` got "must be an integer >= 1" while +/// `line: -5` got "must be a non-negative integer", a rule that explicitly +/// PERMITS the value the other one rejects. A caller cannot correct against +/// guidance that disagrees with itself. +#[test] +fn one_based_arguments_have_a_single_consistent_rule() { + let responses = run_lines(&[ + call_line(1, "goto_line", json!({ "line": 0 })), + call_line(2, "goto_line", json!({ "line": -5 })), + call_line(3, "goto_line", json!({ "line": 1.5 })), + call_line(4, "goto_line", json!({ "line": "3" })), + ]); + + for r in &responses { + assert_eq!(r["error"]["code"], -32602, "expected -32602 in {r}"); + } + + let msgs: Vec = responses + .iter() + .map(|r| { + r["error"]["message"] + .as_str() + .unwrap_or_default() + .to_string() + }) + .collect(); + + // Vacuity guard: prove the messages are real before comparing them, or an + // all-empty result set would satisfy the equality check below for free. + assert!( + msgs.iter().all(|m| m.contains("line")), + "expected each message to name the offending key, got {msgs:?}" + ); + assert!( + msgs.iter().all(|m| m.contains(">= 1")), + "every 1-based rejection must state the >= 1 rule, got {msgs:?}" + ); + assert!( + !msgs.iter().any(|m| m.contains("non-negative")), + "\"non-negative\" permits 0, which is exactly what this rejects: {msgs:?}" + ); + assert!( + msgs.windows(2).all(|w| w[0] == w[1]), + "the same mistake must produce the same message, got {msgs:?}" + ); +} + // --------------------------------------------------------------------------- // Resources (spec 2025-06-18) // --------------------------------------------------------------------------- diff --git a/resources/notepatra.rc b/resources/notepatra.rc.in similarity index 64% rename from resources/notepatra.rc rename to resources/notepatra.rc.in index 3c4be1d..4672b1b 100644 --- a/resources/notepatra.rc +++ b/resources/notepatra.rc.in @@ -1,15 +1,20 @@ -// Windows resource script for Notepatra. +// Windows resource script for Notepatra — GENERATED from +// resources/notepatra.rc.in by CMake. Do not edit the generated copy. +// +// Until v0.1.125 this was a STATIC file carrying a hardcoded 0,1,0,0, +// so every Windows build ever shipped reported version 0.1.0 to winget, +// SCCM, Intune and File Properties, no matter what it actually was. // CMakeLists.txt picks this up via add_executable() on Windows targets and // passes it to MSVC's resource compiler. // IDI_ICON1 is what Windows Explorer uses as the .exe icon (lowest numbered // icon resource wins). Path is relative to this .rc file's location. -IDI_ICON1 ICON "notepatra.ico" +IDI_ICON1 ICON "@NOTEPATRA_ICON_PATH@" // Version info -- shows up in File Properties > Details on Windows. 1 VERSIONINFO -FILEVERSION 0,1,0,0 -PRODUCTVERSION 0,1,0,0 +FILEVERSION @NOTEPATRA_RC_VERSION_COMMA@ +PRODUCTVERSION @NOTEPATRA_RC_VERSION_COMMA@ FILEFLAGSMASK 0x3fL FILEFLAGS 0x0L FILEOS 0x40004L // VOS_NT_WINDOWS32 @@ -22,12 +27,12 @@ BEGIN BEGIN VALUE "CompanyName", "Prateek Singh" VALUE "FileDescription", "Notepatra native code editor for the AI era" - VALUE "FileVersion", "0.1.0.0" + VALUE "FileVersion", "@NOTEPATRA_RC_VERSION_DOT@" VALUE "InternalName", "notepatra" VALUE "LegalCopyright", "Copyright 2026 Prateek Singh. GPL-3.0." VALUE "OriginalFilename", "notepatra.exe" VALUE "ProductName", "Notepatra" - VALUE "ProductVersion", "0.1.0" + VALUE "ProductVersion", "@PROJECT_VERSION@" END END BLOCK "VarFileInfo" diff --git a/src/ai_tools.cpp b/src/ai_tools.cpp index 56d83c7..9320fdf 100644 --- a/src/ai_tools.cpp +++ b/src/ai_tools.cpp @@ -2,6 +2,8 @@ #include "ai_tools.h" +#include "path_denylist.h" + #include "csvanalyst.h" #include "dbconnections.h" #include "git_tools.h" @@ -47,65 +49,12 @@ namespace Limits { // ═══════════════════════════════════════════════════════════════════════ bool isHardDenied(const QString &absPath) { - const QString p = absPath.toLower(); - - // Substring matches — credential / secret directories. Both forward- - // and back-slash variants so this works on Windows where canonical - // paths sometimes preserve backslashes from the OS API. Match on - // path SEGMENTS (with leading + trailing separators) so we don't - // false-positive on a project named "ssh" at the workspace root. - static const QStringList denyContains = { - "/.ssh/", "\\.ssh\\", - "/.gnupg/", "\\.gnupg\\", - "/.aws/", "\\.aws\\", - "/.netrc", "\\.netrc", - "/etc/passwd", "/etc/shadow", - "/.npmrc", // npm authToken - "/.pypirc", // pypi credentials - "/.docker/config.json", - }; - for (const QString &needle : denyContains) { - if (p.contains(needle)) return true; - } - - // Suffix matches — private-key file extensions. *.pem and *.key are - // not always credentials (test fixtures sometimes use them) but the - // false-positive rate is low enough that "refuse and explain" is - // safer than "leak occasionally". - // - // v0.1.106: add Terraform variable / state files — *.tfvars routinely - // hold provider credentials and *.tfstate embeds resource secrets in - // plaintext. - static const QStringList denySuffix = { - ".pem", ".key", ".pfx", ".p12", ".tfvars", ".tfstate", - }; - for (const QString &suf : denySuffix) { - if (p.endsWith(suf)) return true; - } - - // Filename / segment patterns — id_rsa, id_ed25519, id_ecdsa, etc. - // Match on a leading path separator so a project named "secretsjson" - // or "myenv" at the workspace root doesn't false-positive. - // - // v0.1.106: add the dotenv family and secrets.json — the original - // deny-list never covered them, so read_file(".env") leaked its body - // verbatim to the backend. - static const QStringList denyFilename = { - "id_rsa", "id_ed25519", "id_ecdsa", "id_dsa", - "authorized_keys", "known_hosts", - ".env", "secrets.json", - }; - for (const QString &name : denyFilename) { - if (p.contains("/" + name) || p.contains("\\" + name)) return true; - } - - // v0.1.106: the dotenv convention also covers .env (app.env, - // database.env, prod.env) and *.env. (app.env.bak), which the - // leading-"/.env" match above misses. Any path ending in ".env" or - // containing ".env." is overwhelmingly credential-bearing — deny it. - if (p.endsWith(".env") || p.contains(".env.")) return true; - - return false; + // Delegates to PathDenylist — see src/path_denylist.h. This list used to + // live here AND in git_hunk_apply.cpp, and the two had drifted (this copy + // lacked *.jks; that one lacked the dotenv family, *.tfvars and the + // Windows separator variants). search_project consulted neither and could + // return matching LINES out of ~/.ssh/. One list, three callers. + return PathDenylist::isSecretPath(absPath); } bool resolveSafePath(const QString &pathArg, diff --git a/src/aipanel.cpp b/src/aipanel.cpp index a1182d6..6fc9d3c 100644 --- a/src/aipanel.cpp +++ b/src/aipanel.cpp @@ -3395,9 +3395,10 @@ void AIPanel::sendPrompt(const QString &action) { if (codingNow && m_workspaceRoot.isEmpty()) { appendErrorBubble( "Coding Mode needs an open folder so I can read/edit your " - "files. Open one via File → Open Folder… (or drag a folder " - "onto the window), then ask again. If you just want to chat " - "about code in general (no file ops), switch to Chat mode."); + "files. Open one via File → Open Folder as Workspace... (or " + "drag a folder onto the window), then ask again. If you just " + "want to chat about code in general (no file ops), switch to " + "Chat mode."); return; } if (dataNow && DbConnections::loadAll().isEmpty()) { @@ -6985,6 +6986,34 @@ void AIPanel::updateChatHistoryPath() { const QByteArray hash = QCryptographicHash::hash( m_workspaceRoot.toUtf8(), QCryptographicHash::Sha1).toHex(); m_chatHistoryPath = historyDir + "/" + QString::fromLatin1(hash) + ".json"; + + // v0.1.125 migration — adopt history stranded under sha1($HOME). + // + // Until this release the workspace root silently fell back to the file + // tree's display folder, which defaults to the home directory. So for every + // user who never opened a folder, their entire conversation history was + // written under sha1($HOME). Now that the root is derived correctly, that + // file is unreachable and the AI panel looks like it forgot everything. + // + // Adopt it ONCE, and only when the new location does not already exist, so + // this can never overwrite a real conversation. Copy rather than move: if + // anything here is wrong the original is still on disk, and a stale extra + // file costs the user nothing while a deleted one costs them their history. + if (QFileInfo::exists(m_chatHistoryPath)) return; + + const QString homeRoot = QDir::homePath(); + if (m_workspaceRoot == homeRoot) return; // already the legacy key + + const QByteArray legacyHash = QCryptographicHash::hash( + homeRoot.toUtf8(), QCryptographicHash::Sha1).toHex(); + const QString legacyPath = + historyDir + "/" + QString::fromLatin1(legacyHash) + ".json"; + if (!QFileInfo::exists(legacyPath)) return; + + if (QFile::copy(legacyPath, m_chatHistoryPath)) { + qInfo("chat history: adopted pre-v0.1.125 history from %s", + qUtf8Printable(QFileInfo(legacyPath).fileName())); + } } void AIPanel::scheduleChatSave() { diff --git a/src/editor.cpp b/src/editor.cpp index 304a1ba..e7b6ee3 100644 --- a/src/editor.cpp +++ b/src/editor.cpp @@ -1335,9 +1335,21 @@ void Editor::onMarginClicked(int margin, int line, Qt::KeyboardModifiers) { } } -void Editor::gotoLine(int line) { - setCursorPosition(line - 1, 0); - ensureLineVisible(line - 1); +// Jump to a 1-based line, CLAMPED to the buffer. Returns the line actually +// landed on, so callers can report the truth rather than echo the request. +// +// Unclamped, an out-of-range line was worse than a no-op: Scintilla rejects the +// position and leaves the cursor at 0, so asking for line 99999 in a 4-line file +// silently moved the cursor to the TOP. Over MCP that response still said +// ok:true, and insert_text defaults to the cursor — so an assistant aiming at +// the end of a file wrote at the beginning, with an approval card that looked +// entirely legitimate. +int Editor::gotoLine(int line) { + const int total = lines(); + const int target = qBound(1, line, total > 0 ? total : 1); + setCursorPosition(target - 1, 0); + ensureLineVisible(target - 1); + return target; } void Editor::duplicateLine() { diff --git a/src/editor.h b/src/editor.h index e336af6..cf40c5c 100644 --- a/src/editor.h +++ b/src/editor.h @@ -95,7 +95,8 @@ class Editor : public QsciScintilla { struct CommentSyntax { QString line; QString blockOpen; QString blockClose; }; static CommentSyntax commentSyntaxFor(const QString &lang); - void gotoLine(int line); + // Clamped to [1, lines()]; returns the line actually landed on. + int gotoLine(int line); void updateGitGutter(); void duplicateLine(); void deleteLine(); diff --git a/src/fileexplorer.cpp b/src/fileexplorer.cpp index 681c147..9a4b37a 100644 --- a/src/fileexplorer.cpp +++ b/src/fileexplorer.cpp @@ -99,6 +99,9 @@ class FileExplorer::HiddenPathProxy : public QSortFilterProxyModel { }; FileExplorer::FileExplorer(QWidget *parent) : QWidget(parent) { + // Display placeholder ONLY — deliberately leaves m_rootExplicit false, so + // workspaceRoot() stays empty until the user opens a folder. Do not treat + // this as a workspace; see the comment on workspaceRoot() in the header. m_rootPath = QDir::homePath(); auto *layout = new QVBoxLayout(this); @@ -143,13 +146,29 @@ FileExplorer::FileExplorer(QWidget *parent) : QWidget(parent) { m_tree->setStyle(new SsmsBranchStyle(qApp->style())); layout->addWidget(m_tree); - connect(m_pathCombo, &QComboBox::currentTextChanged, this, [this](const QString &path) { - if (QFileInfo(path).isDir()) { - m_rootPath = path; - m_model->setRootPath(path); - m_tree->setRootIndex(m_proxy->mapFromSource(m_model->index(path))); - } - }); + // Navigate on COMMIT only — a dropdown pick or Enter — never on + // currentTextChanged. + // + // currentTextChanged fires on every keystroke of an editable combo, so + // typing a path re-rooted the tree once per character AND, worse, latched + // m_rootExplicit on whatever prefix happened to be a directory. Typing a + // single "/" was enough to make "/" the user's "explicit workspace" + // permanently — which then anchors the AI file sandbox at the filesystem + // root and hands search_project the whole disk. The flag has no way back. + // + // Both signals below are user-commit events; neither can fire mid-typing. + auto navigateTo = [this](const QString &path) { + if (!QFileInfo(path).isDir()) return; + m_rootPath = path; + m_rootExplicit = true; // a deliberate, committed choice + m_model->setRootPath(path); + m_tree->setRootIndex(m_proxy->mapFromSource(m_model->index(path))); + }; + connect(m_pathCombo, QOverload::of(&QComboBox::activated), this, + [this, navigateTo](int) { navigateTo(m_pathCombo->currentText()); }); + if (auto *edit = m_pathCombo->lineEdit()) + connect(edit, &QLineEdit::returnPressed, this, + [this, navigateTo] { navigateTo(m_pathCombo->currentText()); }); connect(upBtn, &QPushButton::clicked, this, [this]() { QDir dir(m_rootPath); @@ -204,6 +223,7 @@ FileExplorer::FileExplorer(QWidget *parent) : QWidget(parent) { void FileExplorer::setRoot(const QString &path) { if (QFileInfo(path).isDir()) { m_rootPath = path; + m_rootExplicit = true; // Open Folder / restored workspace m_pathCombo->setCurrentText(QDir::toNativeSeparators(path)); } } diff --git a/src/fileexplorer.h b/src/fileexplorer.h index f363033..91743f2 100644 --- a/src/fileexplorer.h +++ b/src/fileexplorer.h @@ -17,10 +17,24 @@ class FileExplorer : public QWidget { public: explicit FileExplorer(QWidget *parent = nullptr); void setRoot(const QString &path); - // Expose the current root so MainWindow can seed the AI workspace - // context from it (so the AI knows *which* folder to reason about). + // The folder the TREE is currently displaying. Never empty — it starts at + // the home directory so the widget has something to render. Use this only + // for view concerns. QString rootPath() const { return m_rootPath; } + // The folder the user EXPLICITLY opened, or "" if they never opened one. + // + // These two are different questions and conflating them caused a privacy + // bug: search_project treated the display root as a workspace, so with no + // folder open it walked the user's entire home directory and returned + // line-level content from files they had never opened — including, in the + // report that found this, the transcript of the session driving the tool. + // Anything that decides "what is the user working on" MUST use this one, + // and must treat "" as "nothing is scoped", not as "start at home". + QString workspaceRoot() const { + return m_rootExplicit ? m_rootPath : QString(); + } + // v0.1.61 — hidden-path filter. Right-click any tree node to add it // to the hidden set; "Show hidden" empties the set. Persisted across // sessions via Config::explorerHiddenPaths so users don't have to @@ -40,6 +54,9 @@ class FileExplorer : public QWidget { QTreeView *m_tree; QComboBox *m_pathCombo; QString m_rootPath; + // False until the user actually picks a folder. The ctor's home-directory + // seed does NOT set it — that is a placeholder for the view, not a choice. + bool m_rootExplicit = false; // v0.1.61 — proxy that filters out anything in m_hiddenPaths so the // tree view doesn't render hidden entries. Operates on absolute file diff --git a/src/git_hunk_apply.cpp b/src/git_hunk_apply.cpp index 1cb246d..2ee0f32 100644 --- a/src/git_hunk_apply.cpp +++ b/src/git_hunk_apply.cpp @@ -2,6 +2,8 @@ #include "git_hunk_apply.h" +#include "path_denylist.h" + #include #include #include @@ -49,24 +51,12 @@ bool inlineResolveReadPath(const QString &absFilePath, return false; } // Hardcoded deny-list — never stage hunks against credentials / keys. - // Mirrors ai_tools.cpp::isHardDenied. - const QString lower = fileCanonical.toLower(); - static const QStringList denyExt = {".pem", ".key", ".p12", ".pfx", ".jks"}; - for (const QString &e : denyExt) { - if (lower.endsWith(e)) { - if (outErrorKind) *outErrorKind = "denied"; - return false; - } - } - static const QStringList denySubstr = { - "/.ssh/", "/.gnupg/", "/.aws/", "/.netrc", "/.npmrc", - "/.docker/config.json", "id_rsa", "/etc/passwd", "/etc/shadow" - }; - for (const QString &s : denySubstr) { - if (lower.contains(s)) { - if (outErrorKind) *outErrorKind = "denied"; - return false; - } + // Shared with ai_tools.cpp and search_project via PathDenylist; this used + // to be a hand-maintained MIRROR of the ai_tools list and had already + // drifted from it in both directions. + if (PathDenylist::isSecretPath(fileCanonical)) { + if (outErrorKind) *outErrorKind = "denied"; + return false; } if (outCanonical) *outCanonical = fileCanonical; return true; diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 564f214..a299dde 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1321,8 +1321,25 @@ MainWindow::MainWindow(bool standaloneNoSession) // (or file) so the AI has something concrete to read/edit. One- // shot per session via m_codingFolderPromptShown — re-entering // Coding mode after dismissing doesn't re-pester. - if (on && !m_codingFolderPromptShown - && m_explorer->rootPath().isEmpty()) { + // Fire ONLY when the user genuinely has nothing to work on: no folder + // opened AND not a single file-backed tab anywhere. + // + // The tab scan matters: with a Terminal or Welcome tab focused, any + // "current file" check reads empty even though real files are open one + // tab over, and the dialog would contradict itself by demanding "a + // folder or file" while both are already there. + // + // Worth stating plainly: this prompt has never once fired in a shipped + // build. Its old gate was FileExplorer::rootPath().isEmpty(), and that + // defaults to $HOME, so the condition was permanently false. Fixing the + // workspace-root bug is what woke it up. + bool anyFileOpen = false; + for (int i = 0; m_tabs && i < m_tabs->count() && !anyFileOpen; ++i) + if (auto *ed = m_tabs->editorAt(i)) + anyFileOpen = !ed->filePath().isEmpty(); + + if (on && !m_codingFolderPromptShown && !anyFileOpen + && workspaceFolder().isEmpty()) { m_codingFolderPromptShown = true; QMessageBox box(this); box.setWindowTitle(tr("Coding Mode — open something to work on")); @@ -1769,18 +1786,11 @@ MainWindow::MainWindow(bool standaloneNoSession) auto *ed = currentEditor(); return ed ? ed->selectedText() : QString(); }; - // Explorer root, else the current file's directory (git resolves the - // enclosing repo from there); "" only when neither exists. - auto effectiveWorkspaceRoot = [this]() -> QString { - QString root = m_explorer ? m_explorer->rootPath() : QString(); - if (root.isEmpty()) { - if (auto *ed = currentEditor()) - if (!ed->filePath().isEmpty()) - root = QFileInfo(ed->filePath()).absolutePath(); - } - return root; - }; - host.workspaceRoot = effectiveWorkspaceRoot; + // STRICT folder-only. NOT a current-file fallback: with a single file + // open in $HOME that fallback resolves to $HOME, and search_project + // walks the user's entire profile — the exact leak this release fixes, + // reached through a one-file door instead of a no-file door. + host.workspaceRoot = [this] { return workspaceFolder(); }; // ── v0.1.118 expansive wave — every lambda routes through the SAME // code path the equivalent menu/status-bar surface uses. ── host.currentTabIndex = [this] { return m_tabs->currentIndex(); }; @@ -1811,14 +1821,15 @@ MainWindow::MainWindow(bool standaloneNoSession) } return m_tabs->currentIndex(); // newFile() focuses the new tab }; - host.gotoLine = [this](int tabIndex, int line) { - if (tabIndex < 0 || tabIndex >= m_tabs->count()) return false; + host.gotoLine = [this](int tabIndex, int line) -> int { + if (tabIndex < 0 || tabIndex >= m_tabs->count()) return -1; auto *ed = m_tabs->editorAt(tabIndex); - if (!ed) return false; + if (!ed) return -1; m_tabs->setCurrentIndex(tabIndex); - ed->gotoLine(line); // 1-based; ensures the line is visible + // 1-based; clamps to the buffer and ensures the line is visible. + const int landed = ed->gotoLine(line); ed->setFocus(); - return true; + return landed; }; // v0.1.121 (issue #5): move the selection to a 1-based range. Cols are // clamped to each line's text length (EOL excluded) so an over-long @@ -2025,14 +2036,13 @@ MainWindow::MainWindow(bool standaloneNoSession) return arr; }; // READ: read-only git — reuses git_tools.cpp (no new QProcess path). - host.runGit = [this, effectiveWorkspaceRoot]( - const QString &sub, const QJsonObject &args, - QString *err) -> QString { + host.runGit = [this](const QString &sub, const QJsonObject &args, + QString *err) -> QString { // Candidate roots: the workspace (Explorer) folder first, then the // current file's directory — so git "just works" on an open repo // file even when the workspace folder isn't itself a repository. QStringList roots; - const QString wsRoot = effectiveWorkspaceRoot(); + const QString wsRoot = workspaceFolder(); if (!wsRoot.isEmpty()) roots << wsRoot; if (auto *ed = currentEditor()) if (!ed->filePath().isEmpty()) { @@ -2135,7 +2145,7 @@ MainWindow::MainWindow(bool standaloneNoSession) // Deliberately NOT the workspaceRoot fallback: csv sandbox root // stays folder-open-only. const QString wsRoot = - m_explorer ? m_explorer->rootPath() : QString(); + m_explorer ? m_explorer->workspaceRoot() : QString(); QString csvCanon; if (!AiTools::resolveSafePath(csvPath, wsRoot, &csvCanon, nullptr) @@ -2533,6 +2543,44 @@ Editor *MainWindow::currentEditor() { return m_tabs->currentEditor(); } +// See the header for why these are four functions and not one. + +QString MainWindow::workspaceFolder() const { + return m_explorer ? m_explorer->workspaceRoot() : QString(); +} + +QString MainWindow::firstOpenFileDir() const { + for (int i = 0; m_tabs && i < m_tabs->count(); ++i) + if (auto *ed = m_tabs->editorAt(i)) + if (!ed->filePath().isEmpty()) + return QFileInfo(ed->filePath()).absolutePath(); + return QString(); +} + +QString MainWindow::suggestedDialogFolder() const { + const QString folder = workspaceFolder(); + if (!folder.isEmpty()) return folder; + if (auto *ed = m_tabs ? m_tabs->currentEditor() : nullptr) + if (!ed->filePath().isEmpty()) + return QFileInfo(ed->filePath()).absolutePath(); + return firstOpenFileDir(); +} + +QString MainWindow::aiWorkspaceRoot() { + // An explicit folder always wins and re-latches — that IS the user saying + // "this is my project now", and re-keying the conversation is correct there. + const QString folder = workspaceFolder(); + if (!folder.isEmpty()) { + m_aiWorkspaceLatched = folder; + return folder; + } + // Otherwise keep whatever we first settled on. Deriving this from the + // CURRENT tab made the root flap on every Ctrl+Tab. + if (m_aiWorkspaceLatched.isEmpty()) + m_aiWorkspaceLatched = firstOpenFileDir(); + return m_aiWorkspaceLatched; +} + // ── File operations ── Editor *MainWindow::newFile() { @@ -3735,8 +3783,8 @@ void MainWindow::buildMenus() { QString defaultFolder; if (auto *e = E(); e && !e->filePath().isEmpty()) defaultFolder = QFileInfo(e->filePath()).path(); - else if (m_explorer && !m_explorer->rootPath().isEmpty()) - defaultFolder = m_explorer->rootPath(); + else if (m_explorer && !m_explorer->workspaceRoot().isEmpty()) + defaultFolder = m_explorer->workspaceRoot(); if (!defaultFolder.isEmpty()) ps->setFolder(defaultFolder); connect(ps, &ProjectSearch::openFileAtLine, this, @@ -6434,7 +6482,21 @@ void MainWindow::dragEnterEvent(QDragEnterEvent *event) { void MainWindow::dropEvent(QDropEvent *event) { for (const QUrl &url : event->mimeData()->urls()) { - if (url.isLocalFile()) openFile(url.toLocalFile()); + if (!url.isLocalFile()) continue; + const QString path = url.toLocalFile(); + // A dropped DIRECTORY opens as the workspace, matching what the Coding + // Mode refusal has always told users to do ("drag a folder onto the + // window"). Until v0.1.125 that instruction was false: every drop went + // to openFile(), which rejects a directory — and the refusal only became + // reachable in this release, so the advice had never been exercised. + if (QFileInfo(path).isDir()) { + if (m_explorer) { + Config::instance().noteLastDir(path); + m_explorer->setRoot(path); + } + continue; + } + openFile(path); } } @@ -6713,10 +6775,9 @@ void MainWindow::populateAiContext(AIPanel *panel) { // 2. Directory of the current file // This way the AI reasons about the project the user actually opened, // not just the folder containing whichever file happens to be active. - if (m_explorer && !m_explorer->rootPath().isEmpty()) - workspace = m_explorer->rootPath(); - else if (!curPath.isEmpty()) - workspace = QFileInfo(curPath).absolutePath(); + // Sticky for the session — see aiWorkspaceRoot(). A per-tab root cancelled + // pending write approvals and swapped the chat history on every tab switch. + workspace = aiWorkspaceRoot(); // Walk the workspace root (shallow-but-recursive) to hand the AI a // codebase file listing. Lets the model reference files the user diff --git a/src/mainwindow.h b/src/mainwindow.h index 42a7c68..ae673b9 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -47,6 +47,43 @@ class MainWindow : public QMainWindow { // session.json or destroy its crash evidence. explicit MainWindow(bool standaloneNoSession = false); Editor *currentEditor(); + + // ── Workspace roots ────────────────────────────────────────────────── + // + // These are four DIFFERENT questions and a single answer cannot serve them. + // Collapsing them is what produced the original privacy bug, and collapsing + // them a second time (folder-else-current-file) reproduced it: with one file + // open in $HOME, "the current file's directory" IS $HOME, so a search that + // trusted it walked the whole profile again. + // + // Rule of thumb: anything that SCOPES A SEARCH OR A SANDBOX must use + // workspaceFolder() and must treat "" as a refusal to act — never as a cue + // to fall back to something broader. + + /// The folder the user explicitly opened. "" when they never opened one. + /// The only safe root for scoping a filesystem walk or a sandbox. + QString workspaceFolder() const; + + /// Directory of the FIRST file-backed tab, or "". Answers "does the user + /// have anything concrete open?" without caring which tab has focus — a + /// Terminal or Welcome tab being current must not mean "nothing is open". + QString firstOpenFileDir() const; + + /// Folder to pre-fill a file dialog with. May fall back to the current + /// file's directory: the user SEES and can change this value, so a broad + /// default is a convenience here rather than a silent scope. Never use it + /// to bound a search. + QString suggestedDialogFolder() const; + + /// The AI's project root, STICKY for the session. + /// + /// Latches the first non-empty root and keeps it until the user explicitly + /// opens a folder. It cannot be a function of the focused tab: AIPanel + /// treats any root change as a project switch — cancelling pending write + /// approvals and re-keying chat history by sha1(root) — so a per-tab root + /// made Ctrl+Tab silently swap the user's conversation, even between two + /// files in one repository. + QString aiWorkspaceRoot(); void openFile(const QString &path); SearchResultsPanel *searchResults() { return m_searchResults; } QSplitter *vertSplitter() { return m_vertSplitter; } @@ -274,6 +311,9 @@ class MainWindow : public QMainWindow { // Resets on app restart; within one session, the picker shows once // per Coding entry and then stays out of the way. bool m_codingFolderPromptShown = false; + // Sticky AI project root — see aiWorkspaceRoot(). Latched once, then only + // replaced when the user explicitly opens a different folder. + QString m_aiWorkspaceLatched; // Push current workspace state (all open editor tabs, current file, // selection, workspace root) into an AIPanel so the model can reason // about cross-file questions like Cursor / Copilot. diff --git a/src/mcp_bridge.cpp b/src/mcp_bridge.cpp index ef4065d..48f15c3 100644 --- a/src/mcp_bridge.cpp +++ b/src/mcp_bridge.cpp @@ -55,7 +55,12 @@ // // ACT tier — visible, non-destructive, NO approval card. // new_tab {text?} → {tab_index} -// goto_line {line,tab_index?} → {ok,tab_index,line} +// goto_line {line,tab_index?} → {ok,tab_index,line, +// requested_line,clamped} +// `line` = where the cursor +// LANDED (clamped to the +// buffer); clamped:true when it +// differs from requested_line. // select_range{start_line,start_col,end_line,end_col,tab_index?} // → {ok,tab_index} (1-based line // +col; v0.1.121) @@ -99,6 +104,7 @@ #include "config.h" #include "diagram/npd_parser.h" #include "notes_storage.h" +#include "path_denylist.h" #include "singleinstance.h" #include @@ -253,7 +259,33 @@ QJsonObject scanWorkspace(const QString &root, const QString &query, } const QFileInfo fi = it.fileInfo(); if (fi.size() > kMaxSearchFileBytes) continue; + // Skip VCS / dependency / build trees, the same set populateAiContext + // already excludes. These hold thousands of files that burn the scan + // budget for no user benefit, and .git in particular can echo the + // contents of files the user never opened. + { + const QString rel = QDir(root).relativeFilePath(path); + static const char *kSkipDirs[] = {".git/", "node_modules/", + "build/", "target/", + ".venv/", "__pycache__/"}; + bool skip = false; + for (const char *d : kSkipDirs) { + const QString seg = QLatin1String(d); + if (rel.startsWith(seg) || rel.contains(QLatin1Char('/') + seg)) { + skip = true; + break; + } + } + if (skip) continue; + } if (skipPaths.contains(fi.absoluteFilePath())) continue; + // The AI file tools refuse to READ ~/.ssh/id_rsa, *.pem, .aws/ + // credentials and friends (AiTools::isHardDenied). search_project + // walks the same tree with no such check, so a one-word query could + // return the matching LINES out of exactly those files — the deny-list + // was guarding the front door while this verb held the back one open. + // Same list, same thread-safe pure function, applied per file. + if (PathDenylist::isSecretPath(fi.absoluteFilePath())) continue; QFile f(path); if (!f.open(QIODevice::ReadOnly)) continue; if (f.peek(8192).contains('\0')) continue; // binary-looking @@ -284,6 +316,11 @@ QJsonObject scanWorkspace(const QString &root, const QString &query, QJsonObject result; result[QStringLiteral("results")] = hitsToJson(hits); result[QStringLiteral("truncated")] = truncated; + // Reaching here means the workspace really was walked. Every search_project + // response carries these two fields so the caller never has to infer scope + // from the result count. + result[QStringLiteral("workspace_searched")] = true; + result[QStringLiteral("scope")] = QStringLiteral("tabs_and_workspace"); return result; } @@ -832,11 +869,39 @@ void McpBridge::verbSearchProject(QLocalSocket *client, int id, const QString root = m_host.workspaceRoot ? m_host.workspaceRoot() : QString(); - if (root.isEmpty() || !QFileInfo(root).isDir() || - hits.size() >= maxResults) { + // Whether the workspace leg ran is a property of the WORKSPACE, never of + // whether the query happened to match an open tab. + // + // Gating the "no workspace" signal on `hits.isEmpty()` made the contract + // flip on match luck: miss the open tabs and you got a clear error, hit one + // and you got `{results:[...]}` shaped exactly like a completed project + // search — so a caller could not tell a searched project from an unsearched + // one. The `!isDir` case (folder deleted or renamed under us) was worse + // still: empty results, `truncated:false`, no error at all. + // + // Every response now states which legs actually ran, and the caller can + // trust that field instead of inferring from result count. + const bool haveWorkspace = !root.isEmpty() && QFileInfo(root).isDir(); + if (!haveWorkspace && hits.isEmpty()) { + sendError(client, id, + root.isEmpty() + ? QStringLiteral("No workspace folder is open, so there " + "is nothing to search beyond the open " + "tabs. Open a folder first.") + : QStringLiteral("The workspace folder no longer exists, " + "so only open tabs could be searched. " + "Re-open the folder.")); + return; + } + if (!haveWorkspace || hits.size() >= maxResults) { QJsonObject result; result[QStringLiteral("results")] = hitsToJson(hits); result[QStringLiteral("truncated")] = hits.size() >= maxResults; + // False here means "open tabs only" — the workspace was never walked. + result[QStringLiteral("workspace_searched")] = haveWorkspace; + result[QStringLiteral("scope")] = + haveWorkspace ? QStringLiteral("tabs_and_workspace") + : QStringLiteral("open_tabs_only"); sendResult(client, id, result); return; } @@ -1011,14 +1076,21 @@ void McpBridge::verbGotoLine(QLocalSocket *client, int id, QStringLiteral("tab index out of range: %1").arg(idx)); return; } - if (!m_host.gotoLine(idx, line)) { + const int landed = m_host.gotoLine(idx, line); + if (landed < 1) { sendError(client, id, QStringLiteral("could not move cursor")); return; } QJsonObject result; result[QStringLiteral("ok")] = true; result[QStringLiteral("tab_index")] = idx; - result[QStringLiteral("line")] = line; + // The line ACTUALLY landed on, which is not always the one requested: a + // line past end-of-file clamps to the last line. Echoing the request here + // was a lie that callers acted on — insert_text defaults to the cursor, so + // "ok, line 99999" meant a write at the top of the file. + result[QStringLiteral("line")] = landed; + result[QStringLiteral("requested_line")] = line; + result[QStringLiteral("clamped")] = (landed != line); sendResult(client, id, result); } @@ -1357,6 +1429,18 @@ void McpBridge::verbInsertText(QLocalSocket *client, int id, return; } int line = -1, col = -1; + // `col` alone is meaningless and used to be silently DISCARDED: the insert + // then went to the cursor, so an assistant asking for column 1 of wherever + // it thought it was landed somewhere else entirely — behind an approval + // card whose text said "at the cursor" and was therefore truthful but + // unread. Reject it and say which argument is missing. + if (args.contains(QLatin1String("col")) && + !args.contains(QLatin1String("line"))) { + sendError(client, id, + QStringLiteral("col requires line — pass both, or neither " + "to insert at the cursor")); + return; + } if (args.contains(QLatin1String("line"))) { line = args.value(QLatin1String("line")).toInt(0); col = args.contains(QLatin1String("col")) diff --git a/src/mcp_bridge.h b/src/mcp_bridge.h index 6734edb..7e21cf7 100644 --- a/src/mcp_bridge.h +++ b/src/mcp_bridge.h @@ -47,7 +47,11 @@ struct McpEditorHost { std::function cursorPosition; // 1-based line/col std::function recentFiles; std::function newTab; // initial text ("" = empty) → index, -1 on failure - std::function gotoLine; // (tab index, 1-based line) + // (tab index, 1-based line) → the line ACTUALLY landed on after clamping + // to the buffer, or -1 if the tab could not be targeted at all. Returning + // the clamped line (not a bool) is what lets goto_line report the truth + // instead of echoing a request it did not honour. + std::function gotoLine; std::function setLanguage; // false = unknown language std::function compareTabs; // opens the Compare dialog (non-modal) std::function diff --git a/src/path_denylist.cpp b/src/path_denylist.cpp new file mode 100644 index 0000000..39783cf --- /dev/null +++ b/src/path_denylist.cpp @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +#include "path_denylist.h" + +#include + +namespace PathDenylist { + +bool isSecretPath(const QString &absPath) { + const QString p = absPath.toLower(); + + // Segment matches — credential directories and system password files. + // Both forward- and back-slash variants, because canonical paths on + // Windows sometimes preserve the separators the OS API handed back. + // Matching on SEGMENTS (leading + trailing separator) keeps a project + // folder legitimately named "ssh" or "aws" out of the deny-list. + static const QStringList kSegments = { + QStringLiteral("/.ssh/"), QStringLiteral("\\.ssh\\"), + QStringLiteral("/.gnupg/"), QStringLiteral("\\.gnupg\\"), + QStringLiteral("/.aws/"), QStringLiteral("\\.aws\\"), + QStringLiteral("/.netrc"), QStringLiteral("\\.netrc"), + QStringLiteral("/.npmrc"), QStringLiteral("/.pypirc"), + QStringLiteral("/.docker/config.json"), + QStringLiteral("/etc/passwd"), QStringLiteral("/etc/shadow"), + }; + for (const QString &needle : kSegments) + if (p.contains(needle)) return true; + + // Extension matches — private keys, keystores, and Terraform files. + // *.pem / *.key are not ALWAYS credentials (test fixtures use them), but + // the false-positive cost is one refused read and the false-negative cost + // is a leaked key, so this errs toward refusing. + // + // .jks came from the git-hunk-apply copy of this list and was missing from + // the AI-tools copy — the exact drift this file exists to end. + static const QStringList kSuffixes = { + QStringLiteral(".pem"), QStringLiteral(".key"), + QStringLiteral(".pfx"), QStringLiteral(".p12"), + QStringLiteral(".jks"), QStringLiteral(".tfvars"), + QStringLiteral(".tfstate"), + }; + for (const QString &suffix : kSuffixes) + if (p.endsWith(suffix)) return true; + + // Filename patterns, separator-anchored so "secretsjson" or "myenv" at + // the workspace root does not false-positive. + static const QStringList kFilenames = { + QStringLiteral("id_rsa"), QStringLiteral("id_ed25519"), + QStringLiteral("id_ecdsa"), QStringLiteral("id_dsa"), + QStringLiteral("authorized_keys"), QStringLiteral("known_hosts"), + QStringLiteral(".env"), QStringLiteral("secrets.json"), + }; + for (const QString &name : kFilenames) + if (p.contains(QLatin1Char('/') + name) || + p.contains(QLatin1Char('\\') + name)) + return true; + + // Unanchored id_rsa, kept from the git-hunk-apply list: an SSH key copied + // into a build directory as `backup-id_rsa` is still an SSH key. Dropping + // it in favour of the anchored form above would have narrowed a guard that + // already shipped, so the union keeps both. + if (p.contains(QStringLiteral("id_rsa"))) return true; + + // The dotenv convention also covers .env (app.env, prod.env) and + // *.env. (app.env.bak), which the anchored "/.env" match misses. + if (p.endsWith(QStringLiteral(".env")) || + p.contains(QStringLiteral(".env."))) + return true; + + return false; +} + +} // namespace PathDenylist diff --git a/src/path_denylist.h b/src/path_denylist.h new file mode 100644 index 0000000..0bf4624 --- /dev/null +++ b/src/path_denylist.h @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +#pragma once + +#include + +// Single source of truth for "this path holds credentials — never read it, +// never write it, never quote its contents back to a model". +// +// This list existed in TWO places and they had drifted apart, each covering +// holes the other left open: ai_tools.cpp knew about *.tfvars / *.tfstate / +// .pypirc and Windows backslash separators; git_hunk_apply.cpp knew about +// id_rsa and *.jks. Neither knew everything, and search_project consulted +// neither. What lives here is the UNION, checked by every caller. +// +// QtCore only, no state, no allocation beyond the compare — safe to call from +// a QtConcurrent worker thread (search_project's filesystem leg does). +namespace PathDenylist { + +// True when `absPath` looks like a secret store: an SSH/GPG/AWS/Docker +// credential directory, a private-key or Terraform state extension, a netrc / +// npmrc / pypirc, or a system password file. Matched case-insensitively on +// path SEGMENTS so a project directory legitimately named "ssh" is not caught. +bool isSecretPath(const QString &absPath); + +} // namespace PathDenylist diff --git a/test_ai_fullscreen_exit.cpp b/test_ai_fullscreen_exit.cpp index db732cc..9fd9379 100644 --- a/test_ai_fullscreen_exit.cpp +++ b/test_ai_fullscreen_exit.cpp @@ -610,6 +610,93 @@ int main(int argc, char *argv[]) { reopenedW > 50); } + + // ─────────────────────────────────────────────────────────────── + // SCENARIO 18: the AI's project root is STABLE across tab switches. + // + // The AI root used to be recomputed per call as "explorer folder, else + // the CURRENT tab's directory". With no folder open that flapped on every + // Ctrl+Tab: the chat history is keyed on the root, so switching tabs + // silently swapped the conversation out from under the user and cancelled + // any pending write approval. aiWorkspaceRoot() latches once per session + // and only an EXPLICIT folder re-keys it. + // ─────────────────────────────────────────────────────────────── + std::printf("\n-- Scenario 18: AI workspace root stable across tab switches --\n"); + { + QTemporaryDir otherDir; + FileExplorer *explorer = mw.findChild(); + EXPECT_TRUE("S18 setup: FileExplorer located", explorer != nullptr); + EXPECT_TRUE("S18 setup: second temp dir valid", otherDir.isValid()); + + if (explorer && otherDir.isValid()) { + // Vacuity guard. Every "stayed the same" assertion below is + // meaningless if a folder is already open (then the root is + // pinned for the boring reason) or if the root is empty (then + // it trivially never changes). Both must be false to proceed. + EXPECT_TRUE("S18 guard: no explicit workspace folder is open", + mw.workspaceFolder().isEmpty()); + + const QString firstRoot = mw.aiWorkspaceRoot(); + EXPECT_TRUE("S18: root latched to the open file's directory", + firstRoot == workDir.path()); + EXPECT_FALSE("S18 guard: latched root is not empty", + firstRoot.isEmpty()); + + // Open a file living in a DIFFERENT directory and focus it. + const QString fileD = + writeTempFile(otherDir.path(), "d.txt", "delta\n"); + EXPECT_FALSE("S18 setup: wrote d.txt in the other dir", + fileD.isEmpty()); + mw.openFile(fileD); + QApplication::processEvents(); + + // Vacuity guard #2: prove the switch actually LANDED on the new + // directory. Without this, "root unchanged" would pass even if + // openFile had silently failed. + EXPECT_TRUE("S18 guard: current tab is now the other dir's file", + mw.suggestedDialogFolder() == otherDir.path()); + EXPECT_TRUE("S18: AI root UNCHANGED after opening a file " + "elsewhere", + mw.aiWorkspaceRoot() == firstRoot); + + // ...and unchanged again after a plain Ctrl+Tab-style switch. + // + // The switch must END on the OTHER directory's tab. Switching away + // to a workDir tab and asserting there is vacuous: the pre-fix + // recompute-per-call also returns workDir from a workDir tab, so + // the assertion passed against the very bug it exists to catch. + const int otherTab = tabs->currentIndex(); + int homeTab = -1; + for (int i = 0; i < tabs->count(); ++i) { + if (i == otherTab) continue; + tabs->setCurrentIndex(i); + QApplication::processEvents(); + if (mw.suggestedDialogFolder() == workDir.path()) { + homeTab = i; + break; + } + } + EXPECT_TRUE("S18 guard: switched away to a workDir tab", + homeTab >= 0); + tabs->setCurrentIndex(otherTab); // Ctrl+Tab back + QApplication::processEvents(); + EXPECT_TRUE("S18 guard: landed back on the other dir's tab", + tabs->currentIndex() == otherTab + && mw.suggestedDialogFolder() == otherDir.path()); + EXPECT_TRUE("S18: AI root UNCHANGED after a tab switch", + mw.aiWorkspaceRoot() == firstRoot); + + // An EXPLICIT folder is the one thing that may re-key it — that + // is the user saying "this is my project now". + explorer->setRoot(otherDir.path()); + QApplication::processEvents(); + EXPECT_TRUE("S18 guard: explorer now reports an explicit folder", + explorer->workspaceRoot() == otherDir.path()); + EXPECT_TRUE("S18: opening a folder DOES re-latch the AI root", + mw.aiWorkspaceRoot() == otherDir.path()); + } + } + // ─────────────────────────────────────────────────────────────── std::printf("\n=== test_ai_fullscreen_exit: %d passed, %d failed ===\n", g_passed, g_failed); diff --git a/test_mcp_bridge.cpp b/test_mcp_bridge.cpp index ee9b728..3ba3673 100644 --- a/test_mcp_bridge.cpp +++ b/test_mcp_bridge.cpp @@ -162,11 +162,21 @@ class TestMcpBridge : public QObject { m_currentIndex = m_fakeTabs.size() - 1; return m_currentIndex; }; - h.gotoLine = [this](int idx, int line) { - if (idx < 0 || idx >= m_fakeTabs.size() || line < 1) return false; + // Models the REAL Editor::gotoLine contract: clamp to the buffer and + // return the line actually landed on (-1 = could not target the tab). + // + // This stub used to return bool. Under the int-returning host hook that + // still COMPILES — true converts to 1 — so it would have reported + // "landed on line 1" for every successful jump and quietly asserted + // nothing about clamping. A stub that lies is worse than no stub. + h.gotoLine = [this](int idx, int line) -> int { + if (idx < 0 || idx >= m_fakeTabs.size() || line < 1) return -1; + const int total = + qMax(1, m_fakeTabs[idx].text.count(QLatin1Char('\n')) + 1); + const int landed = qBound(1, line, total); m_currentIndex = idx; - m_lastGotoLine = line; - return true; + m_lastGotoLine = landed; + return landed; }; // v0.1.121 (issue #5): flatten the 1-based range to char offsets, set // m_selection to the spanned text so a following replace_selection @@ -1079,6 +1089,199 @@ private slots: QCOMPARE(r.value(QLatin1String("truncated")).toBool(), true); } + + // Scope is a property of the WORKSPACE, never of match luck. + // + // The old guard was `if (root.isEmpty() && hits.isEmpty()) error`. So with + // no folder open, a query that missed the open tabs produced a clear + // "open a folder first" error, while a query that happened to hit one + // produced `{results:[...],truncated:false}` — byte-identical in shape to a + // completed project-wide search. A caller could not distinguish "searched + // your project, found 2" from "searched 2 buffers, never touched disk", + // and would report the second as if it were the first. + // + // Every response now states which legs actually ran. + void search_project_reports_scope_regardless_of_hits() { + QLocalSocket s; + QVERIFY(connectClient(s)); + readGreeting(s); + QVERIFY2(m_root.isEmpty(), "fixture must start with no workspace"); + + // (a) No workspace, query HITS the open tabs → success, but the + // response must admit the workspace leg never ran. + QJsonObject hitArgs; + hitArgs[QStringLiteral("query")] = QStringLiteral("needle"); + const QJsonObject hit = call(s, 60, QStringLiteral("search_project"), + hitArgs); + QVERIFY2(hit.value(QLatin1String("ok")).toBool(), + "an open-tab hit is still a valid result"); + const QJsonObject hr = hit.value(QLatin1String("result")).toObject(); + QVERIFY(hr.value(QLatin1String("results")).toArray().size() > 0); + QCOMPARE(hr.value(QLatin1String("workspace_searched")).toBool(), false); + QCOMPARE(hr.value(QLatin1String("scope")).toString(), + QStringLiteral("open_tabs_only")); + + // (b) No workspace, query misses everything → an actionable error, + // not a silent empty list. + QJsonObject missArgs; + missArgs[QStringLiteral("query")] = + QStringLiteral("zzz-no-such-token-zzz"); + const QJsonObject miss = call(s, 61, QStringLiteral("search_project"), + missArgs); + QCOMPARE(miss.value(QLatin1String("ok")).toBool(), false); + QVERIFY2(miss.value(QLatin1String("error")).toString().contains( + QStringLiteral("No workspace folder")), + "the no-workspace error must say what to do about it"); + } + + // A workspace folder that has been deleted or renamed under us is NOT the + // same as no workspace, and neither one may masquerade as a real search. + // This case used to return `{results:[],truncated:false}` with no error at + // all: an assistant reading that concluded the project contained no match. + void search_project_survives_a_deleted_workspace_root() { + QString gone; + { + QTemporaryDir dir; + QVERIFY(dir.isValid()); + gone = dir.path(); + } // dir removed here + QVERIFY2(!QFileInfo(gone).isDir(), "temp dir must really be gone"); + m_root = gone; + + QLocalSocket s; + QVERIFY(connectClient(s)); + readGreeting(s); + + // With tab hits: succeeds, but must report the reduced scope. + QJsonObject hitArgs; + hitArgs[QStringLiteral("query")] = QStringLiteral("needle"); + const QJsonObject hr = call(s, 62, QStringLiteral("search_project"), + hitArgs) + .value(QLatin1String("result")).toObject(); + QCOMPARE(hr.value(QLatin1String("workspace_searched")).toBool(), false); + QCOMPARE(hr.value(QLatin1String("scope")).toString(), + QStringLiteral("open_tabs_only")); + + // Without tab hits: an error naming the real cause, distinct from the + // "no workspace is open" wording so the two are not confusable. + QJsonObject missArgs; + missArgs[QStringLiteral("query")] = + QStringLiteral("zzz-no-such-token-zzz"); + const QString err = call(s, 63, QStringLiteral("search_project"), + missArgs) + .value(QLatin1String("error")).toString(); + QVERIFY2(err.contains(QStringLiteral("no longer exists")), + qPrintable(QStringLiteral("wrong error for a vanished root: ") + + err)); + } + + // The happy path must claim the workspace leg — otherwise the two fields + // above would be constant-false and prove nothing. + void search_project_claims_the_workspace_when_it_walked_it() { + QTemporaryDir dir; + QVERIFY(dir.isValid()); + { + QFile f(dir.path() + QStringLiteral("/found.txt")); + QVERIFY(f.open(QIODevice::WriteOnly)); + f.write("nothing\nneedle here\n"); + } + m_root = dir.path(); + QLocalSocket s; + QVERIFY(connectClient(s)); + readGreeting(s); + QJsonObject args; + args[QStringLiteral("query")] = QStringLiteral("needle"); + const QJsonObject r = call(s, 64, QStringLiteral("search_project"), + args) + .value(QLatin1String("result")).toObject(); + QCOMPARE(r.value(QLatin1String("workspace_searched")).toBool(), true); + QCOMPARE(r.value(QLatin1String("scope")).toString(), + QStringLiteral("tabs_and_workspace")); + } + + + // search_project must honour the SAME credential deny-list as the AI file + // tools. It walked the workspace with no such check, so `read_file` would + // refuse ~/.ssh/id_rsa while a one-word search_project happily returned the + // matching LINES out of it — the deny-list guarded the front door and this + // verb held the back one open. + void search_project_never_returns_credential_file_contents() { + QTemporaryDir dir; + QVERIFY(dir.isValid()); + const QStringList secrets = { + QStringLiteral("id_rsa"), QStringLiteral("server.pem"), + QStringLiteral(".env"), QStringLiteral("prod.tfvars"), + QStringLiteral("keystore.jks"), + }; + for (const QString &name : secrets) { + QFile f(dir.path() + QLatin1Char('/') + name); + QVERIFY(f.open(QIODevice::WriteOnly)); + f.write("PASSWORD=needle-in-a-secret\n"); + } + // Vacuity guard: an ordinary file with the SAME token must still be + // found, otherwise "no secret hits" could just mean the scan never ran. + { + QFile f(dir.path() + QStringLiteral("/readme.txt")); + QVERIFY(f.open(QIODevice::WriteOnly)); + f.write("harmless needle-in-a-secret mention\n"); + } + m_root = dir.path(); + + QLocalSocket s; + QVERIFY(connectClient(s)); + readGreeting(s); + QJsonObject args; + args[QStringLiteral("query")] = QStringLiteral("needle-in-a-secret"); + const QJsonObject r = call(s, 65, QStringLiteral("search_project"), + args) + .value(QLatin1String("result")).toObject(); + const QJsonArray results = r.value(QLatin1String("results")).toArray(); + + QVERIFY2(r.value(QLatin1String("workspace_searched")).toBool(), + "guard: the workspace leg must actually have run"); + bool sawReadme = false; + for (const QJsonValue &v : results) { + const QJsonObject hit = v.toObject(); + const QString p = hit.value(QLatin1String("path")).toString(); + if (p.endsWith(QLatin1String("readme.txt"))) sawReadme = true; + for (const QString &name : secrets) + QVERIFY2(!p.endsWith(name), + qPrintable(QStringLiteral("leaked contents of ") + + name)); + QVERIFY2(!hit.value(QLatin1String("text")).toString().contains( + QStringLiteral("PASSWORD=")), + "a credential line reached the wire"); + } + QVERIFY2(sawReadme, + "guard: the non-secret file with the same token was missed, " + "so this test proves nothing about the deny-list"); + } + + // `col` without `line` used to be silently DISCARDED — the insert went to + // the cursor instead, behind an approval card reading "at the cursor" that + // was truthful and therefore unremarkable. Silently relocating a write is + // the one thing an approval gate cannot protect the user from. + void insert_text_rejects_col_without_line() { + QLocalSocket s; + QVERIFY(connectClient(s)); + readGreeting(s); + QJsonObject args; + args[QStringLiteral("tab_index")] = 0; + args[QStringLiteral("text")] = QStringLiteral("MARKER-zq7"); + args[QStringLiteral("col")] = 3; // no "line" + const QJsonObject resp = call(s, 66, QStringLiteral("insert_text"), + args); + QCOMPARE(resp.value(QLatin1String("ok")).toBool(), false); + QVERIFY2(resp.value(QLatin1String("error")).toString().contains( + QStringLiteral("col requires line")), + qPrintable(QStringLiteral("wrong error: ") + + resp.value(QLatin1String("error")).toString())); + // ...and it must be rejected BEFORE an approval card is raised, so the + // user is never asked to approve a write that cannot be honoured. + QCOMPARE(m_fakeTabs[0].text.contains(QStringLiteral("MARKER-zq7")), + false); + } + // ── v0.1.118 expansive wave ────────────────────────────────────── void get_status_shape() { @@ -1294,6 +1497,47 @@ private slots: QCOMPARE(bad.value(QLatin1String("ok")).toBool(), false); } + // A line past end-of-file must CLAMP and say so — never claim the requested + // line. Unclamped, Scintilla rejects the position and leaves the cursor at + // the TOP, while the response still read {"line":99999,"ok":true}. Because + // insert_text defaults to the cursor, an assistant aiming at the end of a + // file wrote at the beginning, behind an approval card that looked correct. + void goto_line_past_eof_clamps_and_reports_truth() { + QLocalSocket s; + QVERIFY(connectClient(s)); + readGreeting(s); + + // Establish ground truth from the fake buffer rather than a literal, so + // the test cannot drift if the fixture text changes. + const int total = + qMax(1, m_fakeTabs[1].text.count(QLatin1Char('\n')) + 1); + + QJsonObject args; + args[QStringLiteral("line")] = 99999; + args[QStringLiteral("tab_index")] = 1; + QJsonObject resp = call(s, 37, QStringLiteral("goto_line"), args); + QVERIFY(resp.value(QLatin1String("ok")).toBool()); + const QJsonObject r = resp.value(QLatin1String("result")).toObject(); + + // The cursor really moved to the last line, not to line 1. + QCOMPARE(m_lastGotoLine, total); + // ...and the response reports where it LANDED, not what was asked. + QCOMPARE(r.value(QLatin1String("line")).toInt(), total); + QVERIFY2(r.value(QLatin1String("line")).toInt() != 99999, + "goto_line echoed the requested line instead of the real one"); + QCOMPARE(r.value(QLatin1String("requested_line")).toInt(), 99999); + QCOMPARE(r.value(QLatin1String("clamped")).toBool(), true); + + // An in-range jump must NOT be flagged as clamped. + QJsonObject ok2; + ok2[QStringLiteral("line")] = 1; + ok2[QStringLiteral("tab_index")] = 1; + const QJsonObject r2 = call(s, 38, QStringLiteral("goto_line"), ok2) + .value(QLatin1String("result")).toObject(); + QCOMPARE(r2.value(QLatin1String("line")).toInt(), 1); + QCOMPARE(r2.value(QLatin1String("clamped")).toBool(), false); + } + // v0.1.121 (issue #5): select_range moves the selection (ACT, no card) and // a following replace_selection acts on exactly that span. void select_range_then_replace() { diff --git a/test_workspace_root.cpp b/test_workspace_root.cpp new file mode 100644 index 0000000..d208c89 --- /dev/null +++ b/test_workspace_root.cpp @@ -0,0 +1,148 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +// +// FileExplorer draws a folder tree AND answers "what is the user working on". +// Those are different questions, and answering the second with the first caused +// a privacy bug: search_project over MCP treated the tree's display folder as a +// workspace. With no folder open that folder is $HOME, so a one-word search +// walked the user's entire profile and returned line-level content from files +// they had never opened in the editor — in the report that found this, that +// included the transcript of the session driving the tool. +// +// The same single default silently disarmed three other things that were all +// written to test `rootPath().isEmpty()` and therefore could never fire: +// * the Coding Mode "open a folder" prompt, +// * the AI CSV sandbox root (a SECURITY guard — it was rooted at $HOME +// instead of being closed), +// * the Find-in-Files default folder, whose own comment says walking $HOME +// is "~always wrong". +// +// So the invariant under test is narrow and load-bearing: workspaceRoot() is +// empty until the user actually opens a folder, whatever the tree is showing. + +#include "fileexplorer.h" + +#include +#include +#include +#include +#include +#include + +#include + +static int g_pass = 0, g_fail = 0; +#define EXPECT(label, cond) \ + do { \ + if (cond) { ++g_pass; std::printf(" [PASS] %s\n", label); } \ + else { ++g_fail; std::printf(" [FAIL] %s\n", label); } \ + } while (0) + +int main(int argc, char *argv[]) { + // Isolate config/data writes: QStandardPaths must never touch the real + // profile from a test run. + QTemporaryDir home; + qputenv("XDG_CONFIG_HOME", home.path().toUtf8()); + qputenv("XDG_DATA_HOME", home.path().toUtf8()); + qputenv("QT_QPA_PLATFORM", "offscreen"); + + QApplication app(argc, argv); + + QTemporaryDir workspace; + if (!workspace.isValid()) { + std::printf(" [FAIL] could not create a temp workspace\n"); + return 1; + } + + // ── 1. Fresh explorer: showing something, scoping nothing ───────────── + { + FileExplorer ex; + + // Vacuity guard. If rootPath() were empty the workspaceRoot() assertion + // below would pass for the wrong reason and this test would be worth + // nothing — so prove the display root really is populated first. + EXPECT("precondition: a fresh explorer HAS a display root", + !ex.rootPath().isEmpty()); + EXPECT("precondition: that display root is the home directory", + QDir(ex.rootPath()).absolutePath() == + QDir(QDir::homePath()).absolutePath()); + + // The actual regression guard. + EXPECT("a fresh explorer scopes NO workspace", + ex.workspaceRoot().isEmpty()); + } + + // ── 2. After the user opens a folder, it IS the workspace ───────────── + { + FileExplorer ex; + ex.setRoot(workspace.path()); + + EXPECT("opening a folder sets the workspace root", + !ex.workspaceRoot().isEmpty()); + EXPECT("the workspace root is the folder that was opened", + QDir(ex.workspaceRoot()).absolutePath() == + QDir(workspace.path()).absolutePath()); + EXPECT("display root and workspace root now agree", + QDir(ex.rootPath()).absolutePath() == + QDir(ex.workspaceRoot()).absolutePath()); + } + + // ── 3. A rejected setRoot must not fabricate a workspace ────────────── + // + // setRoot() ignores anything that is not a directory. It must also leave + // the workspace unset, rather than marking it explicit while the path + // silently stays at $HOME — that would reintroduce the original bug + // through a different door. + { + FileExplorer ex; + ex.setRoot(workspace.path() + QStringLiteral("/does-not-exist")); + EXPECT("a non-existent path does not become the workspace", + ex.workspaceRoot().isEmpty()); + + const QString file = workspace.path() + QStringLiteral("/a-file.txt"); + QFile f(file); + if (f.open(QIODevice::WriteOnly)) { f.write("x"); f.close(); } + ex.setRoot(file); + EXPECT("a FILE does not become the workspace", + ex.workspaceRoot().isEmpty()); + } + + // ── 4. Typing must NOT latch a workspace ────────────────────────────── + // + // The path box is an editable QComboBox. Latching on currentTextChanged + // meant every KEYSTROKE committed a workspace: typing a path that passes + // through "/" made "/" the user's explicit workspace, permanently, which + // then anchors the AI file sandbox at the filesystem root and hands + // search_project the whole disk. Only a committed choice may latch. + { + FileExplorer ex; + auto *combo = ex.findChild(); + EXPECT("precondition: the path box is an editable combo", + combo != nullptr && combo->isEditable()); + if (combo && combo->lineEdit()) { + // Clear first: the box is pre-filled with the home path, so typing + // straight in appends and yields a path that does not exist — which + // would make the "does not latch" assertion pass for the wrong + // reason and prove nothing. + combo->lineEdit()->clear(); + combo->lineEdit()->setFocus(); + + // Type a REAL directory one character at a time. Every intermediate + // prefix that happens to be a directory (notably the leading "/") + // used to latch immediately. + QTest::keyClicks(combo->lineEdit(), workspace.path()); + EXPECT("precondition: the typed text really is an existing dir", + QFileInfo(combo->lineEdit()->text()).isDir()); + EXPECT("typing alone does not latch a workspace", + ex.workspaceRoot().isEmpty()); + + // Committing with Enter is a deliberate act, so it may latch. + QTest::keyClick(combo->lineEdit(), Qt::Key_Return); + EXPECT("pressing Enter DOES commit the typed folder", + QDir(ex.workspaceRoot()).absolutePath() == + QDir(workspace.path()).absolutePath()); + } + } + + std::printf("\n%d passed, %d failed\n", g_pass, g_fail); + return g_fail == 0 ? 0 : 1; +} From 3e839fca00d158c4816e9c539642b1cddb0fa6b0 Mon Sep 17 00:00:00 2001 From: Prateek Singh Date: Thu, 6 Aug 2026 22:23:53 -0400 Subject: [PATCH 2/3] fix(mcp): stop the CLI test suite hanging when built with --features remote MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The vacuity guard I added in a0ae9ba spawns each real subcommand and waits for it with .output(). That waits for EOF on the child's pipes, and `serve` built --features remote binds a port and never returns — so the call never came back. CI runs the suite twice, `cargo test --release` then `cargo test --release --features remote`, and the second run wedged the 25-minute step on all four platforms. The C++ suite and the other 91 Rust tests were green everywhere; this one test burned the jobs. It passed locally because I only ever ran the default-feature build, where `serve` exits 2 immediately. Verified the difference directly: default serve -> exit 2 --features remote serve -> still running at 5s Nothing in this file waits unbounded any more. run_bounded() spawns with a null stdin, drains both pipes on their own threads so a full pipe cannot masquerade as a hang, polls try_wait() to a deadline, and kills the child when the budget expires. run() wraps it at 30s and asserts the child exited, so a future regression that makes --help hang fails the test by name instead of timing out the job. For the vacuity guard specifically, a child still running at the 3s budget is now a PASS: the typo guard exits before it can bind anything, so a live process proves the subcommand was understood. If it does exit, the only clean exit is the built-without-remote refusal (2). Red-state verified: moved the dispatch below the guard and dropped its serve|pair|connect exclusion, reproducing the over-broad guard this test exists to catch. Exactly one test went red — the_real_subcommands_are_not_swallowed_by_the_typo_guard, on "unknown subcommand 'serve'" — in 0.10s, not a hang. Both feature configs now pass locally: 4/4 cli tests, cli.rs finishing in 0.10s default and 3.07s with remote (the killed `serve`). clippy 0 in both configs, fmt clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01W6BGY2dFUSujZeS3vTDikq --- notepatra-mcp/tests/cli.rs | 148 ++++++++++++++++++++++++++++--------- 1 file changed, 114 insertions(+), 34 deletions(-) diff --git a/notepatra-mcp/tests/cli.rs b/notepatra-mcp/tests/cli.rs index a1e482c..6a0811c 100644 --- a/notepatra-mcp/tests/cli.rs +++ b/notepatra-mcp/tests/cli.rs @@ -10,40 +10,102 @@ // `sevre` hung too. The rule these tests pin: an argument we do not understand // is a loud exit, never a hang and never a silent fallback. // -// Every child gets a null stdin, so a regression cannot wedge the test suite — -// it would hit EOF and exit 0, which is still a failure against `code == 2`. +// Nothing here may wait on a child unbounded. Some arguments are SUPPOSED to +// block: built `--features remote`, `serve` binds a port and never returns, and +// CI runs the suite twice — once default, once with that feature. The first cut +// of this file called `.output()` on `serve`, which waits for an EOF that never +// comes; it passed locally on the default build and wedged four CI runners for +// 25 minutes each. Every spawn below is time-capped and killed. +use std::io::Read; use std::process::{Command, Stdio}; +use std::time::{Duration, Instant}; -fn run(args: &[&str]) -> (i32, String, String) { - let out = Command::new(env!("CARGO_BIN_EXE_notepatra-mcp")) +struct Outcome { + /// `false` means the child was still running when the budget expired and we + /// killed it. For a subcommand that legitimately blocks, that is a pass. + exited: bool, + code: Option, + stdout: String, + stderr: String, +} + +fn run_bounded(args: &[&str], budget: Duration) -> Outcome { + let mut child = Command::new(env!("CARGO_BIN_EXE_notepatra-mcp")) .args(args) .stdin(Stdio::null()) - .output() + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() .expect("failed to spawn notepatra-mcp"); - ( - out.status.code().unwrap_or(-1), - String::from_utf8_lossy(&out.stdout).into_owned(), - String::from_utf8_lossy(&out.stderr).into_owned(), - ) + + // Drain both pipes on their own threads. A child that fills one of them + // would block before exiting, which would turn the bounded wait below into + // a lie about why it did not finish. + let mut out_pipe = child.stdout.take().expect("stdout was piped"); + let mut err_pipe = child.stderr.take().expect("stderr was piped"); + let out_reader = std::thread::spawn(move || { + let mut s = String::new(); + let _ = out_pipe.read_to_string(&mut s); + s + }); + let err_reader = std::thread::spawn(move || { + let mut s = String::new(); + let _ = err_pipe.read_to_string(&mut s); + s + }); + + let deadline = Instant::now() + budget; + let status = loop { + match child.try_wait().expect("try_wait on the child failed") { + Some(st) => break Some(st), + None if Instant::now() >= deadline => { + let _ = child.kill(); + let _ = child.wait(); + break None; + } + None => std::thread::sleep(Duration::from_millis(25)), + } + }; + + Outcome { + exited: status.is_some(), + code: status.and_then(|s| s.code()), + stdout: out_reader.join().unwrap_or_default(), + stderr: err_reader.join().unwrap_or_default(), + } +} + +/// For arguments that MUST exit. The budget is generous because it should never +/// be reached; when it is, the failure names a hang instead of timing out the job. +fn run(args: &[&str]) -> Outcome { + let o = run_bounded(args, Duration::from_secs(30)); + assert!( + o.exited, + "`{}` hung for 30s — it must exit, not fall into the stdio loop", + args.join(" ") + ); + o } #[test] fn help_and_version_print_and_exit_zero() { for flag in ["-h", "--help"] { - let (code, stdout, _) = run(&[flag]); - assert_eq!(code, 0, "{flag} should exit 0"); + let o = run(&[flag]); + assert_eq!(o.code, Some(0), "{flag} should exit 0"); assert!( - stdout.contains("USAGE"), - "{flag} printed no usage block: {stdout}" + o.stdout.contains("USAGE"), + "{flag} printed no usage block: {}", + o.stdout ); } for flag in ["-V", "--version"] { - let (code, stdout, _) = run(&[flag]); - assert_eq!(code, 0, "{flag} should exit 0"); + let o = run(&[flag]); + assert_eq!(o.code, Some(0), "{flag} should exit 0"); assert!( - stdout.contains(env!("CARGO_PKG_VERSION")), - "{flag} printed no version: {stdout}" + o.stdout.contains(env!("CARGO_PKG_VERSION")), + "{flag} printed no version: {}", + o.stdout ); } } @@ -53,15 +115,17 @@ fn a_mistyped_subcommand_exits_two_instead_of_hanging() { // USAGE advertises bare-word subcommands, so this is a realistic typo — // and it is the half that survived the first fix, which rejected only // flag-shaped arguments. - let (code, _, stderr) = run(&["sevre"]); - assert_eq!(code, 2, "unknown subcommand must exit 2, got {code}"); + let o = run(&["sevre"]); + assert_eq!(o.code, Some(2), "unknown subcommand must exit 2"); assert!( - stderr.contains("unknown subcommand") && stderr.contains("sevre"), - "the error must name what was typed: {stderr}" + o.stderr.contains("unknown subcommand") && o.stderr.contains("sevre"), + "the error must name what was typed: {}", + o.stderr ); assert!( - stderr.contains("serve"), - "the error must list the real subcommands: {stderr}" + o.stderr.contains("serve"), + "the error must list the real subcommands: {}", + o.stderr ); } @@ -69,25 +133,41 @@ fn a_mistyped_subcommand_exits_two_instead_of_hanging() { fn a_mistyped_flag_exits_two_rather_than_starting_a_mock() { // The dangerous half: `--sokcet` used to start the in-memory MOCK, so the // client got fabricated tabs that looked like the user's real editor. - let (code, _, stderr) = run(&["--sokcet"]); - assert_eq!(code, 2, "unknown flag must exit 2, got {code}"); + let o = run(&["--sokcet"]); + assert_eq!(o.code, Some(2), "unknown flag must exit 2"); assert!( - stderr.contains("--sokcet"), - "the error must name the offending flag: {stderr}" + o.stderr.contains("--sokcet"), + "the error must name the offending flag: {}", + o.stderr ); } #[test] fn the_real_subcommands_are_not_swallowed_by_the_typo_guard() { // Vacuity guard for the two tests above: if the guard rejected everything, - // they would pass while the binary was broken. Built without the `remote` - // feature these exit 2 as well, but with a DIFFERENT message — so assert on - // the message, not the code. + // they would pass while the binary was broken. + // + // Two legitimate outcomes, and which one you get depends on the build. A + // std-only build exits 2 with "built without remote support"; a `--features + // remote` build accepts the subcommand and blocks. Still running when the + // budget expires IS the pass — the typo guard exits before it can bind + // anything, so a live process proves the argument was understood. for mode in ["serve", "pair", "connect"] { - let (_, _, stderr) = run(&[mode]); + let o = run_bounded(&[mode], Duration::from_secs(3)); assert!( - !stderr.contains("unknown subcommand"), - "{mode} is a real subcommand and must not hit the typo guard: {stderr}" + !o.stderr.contains("unknown subcommand"), + "{mode} is a real subcommand and must not hit the typo guard: {}", + o.stderr ); + if o.exited { + assert_eq!( + o.code, + Some(2), + "{mode} exited {:?} — the only clean exit here is the \ + built-without-remote refusal: {}", + o.code, + o.stderr + ); + } } } From 0cc3e1959d84f9ca81ec505f635a2be60bdb9190 Mon Sep 17 00:00:00 2001 From: Prateek Singh Date: Thu, 6 Aug 2026 22:39:49 -0400 Subject: [PATCH 3/3] fix(ci): move the PE FileVersion gate below the step that builds the exe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The NP-04 gate ran for the first time on 3e839fc and failed instantly: ##[error]notepatra.exe not found under build/ Not a version mismatch — I had inserted it at step 11 of build-windows, between "Transport test trackers" and "Build C++ with CMake (MSVC)". It asserted on a binary that did not exist yet, so it could only ever fail, and it would have kept failing no matter what the .rc contained. Moved to immediately after "Verify exe, embed icon, bundle Qt + QScintilla DLLs", and retargeted from the build tree to notepatra-win/ — the copy that actually ships, which is the file a user right-clicks -> Properties -> Details on. The bundle step does not touch the resource table (windeployqt and a manual DLL copy only), so this is the same resource the compiler emitted, just checked on the artifact rather than an intermediate. Verified the generation half locally, which is as far as Linux reaches: reconstructed the real WIN32 block from CMakeLists.txt in a throwaway project and ran cmake on it. FILEVERSION 0,1,124,0 PRODUCTVERSION 0,1,124,0 VALUE "FileVersion", "0.1.124.0" VALUE "ProductVersion", "0.1.124" The gate computes "$cmakeVer.0" = "0.1.124.0" and compares against VersionInfo.FileVersion, which reads that StringFileInfo value — exact match, no unsubstituted @VAR@ left, icon path resolves. What remains unverified anywhere: rc.exe actually compiling the generated .rc, and the version surviving into the PE. Only the Windows runner can answer that, and it has still never gotten far enough to try. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01W6BGY2dFUSujZeS3vTDikq --- .github/workflows/build.yml | 46 +++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 20 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 7ffae77..181aea1 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -1219,26 +1219,6 @@ jobs: # on every machine ever. It is now generated from project(Notepatra # VERSION ...) — and this gate is what keeps it that way, because nothing # else on any platform can observe an embedded Windows resource. - - name: Assert PE FileVersion matches the project version - shell: pwsh - run: | - $cmakeVer = (Select-String -Path CMakeLists.txt ` - -Pattern 'project\(Notepatra VERSION ([0-9]+\.[0-9]+\.[0-9]+)' ` - | Select-Object -First 1).Matches.Groups[1].Value - if (-not $cmakeVer) { Write-Host "::error::could not read version from CMakeLists.txt"; exit 1 } - $expected = "$cmakeVer.0" - $exe = Get-ChildItem -Path build -Filter notepatra.exe -Recurse -ErrorAction SilentlyContinue | Select-Object -First 1 - if (-not $exe) { Write-Host "::error::notepatra.exe not found under build/"; exit 1 } - $actual = (Get-Item $exe.FullName).VersionInfo.FileVersion - Write-Host "CMakeLists version : $cmakeVer" - Write-Host "expected FileVersion: $expected" - Write-Host "actual FileVersion: $actual" - if ($actual -ne $expected) { - Write-Host "::error::PE FileVersion '$actual' != expected '$expected' — the .rc template did not substitute" - exit 1 - } - Write-Host "OK: PE version resource matches the project version" - # Separate step because the assertion needs pwsh string matching, and cmd # has no clean equivalent. `serve` on a std-only build exits 2 after # printing this, so we match on the text rather than the exit code. @@ -1498,6 +1478,32 @@ jobs: Copy-Item "installers\unregister-associations.bat" ".\notepatra-win\" -Force } + # MUST stay AFTER the bundle step. This asserts on a built artifact; + # sitting above "Build C++ with CMake" it failed every run with + # "not found" on a binary that did not exist yet. + - name: Assert PE FileVersion matches the project version + shell: pwsh + run: | + $cmakeVer = (Select-String -Path CMakeLists.txt ` + -Pattern 'project\(Notepatra VERSION ([0-9]+\.[0-9]+\.[0-9]+)' ` + | Select-Object -First 1).Matches.Groups[1].Value + if (-not $cmakeVer) { Write-Host "::error::could not read version from CMakeLists.txt"; exit 1 } + $expected = "$cmakeVer.0" + # The SHIPPED exe, not the one still in the build tree: the bundle + # step copies it out and embeds the icon, so this is the file a + # user right-clicks -> Properties -> Details on. + $exe = Get-ChildItem -Path notepatra-win -Filter notepatra.exe -ErrorAction SilentlyContinue | Select-Object -First 1 + if (-not $exe) { Write-Host "::error::notepatra.exe not found in notepatra-win/ - did the bundle step run?"; exit 1 } + $actual = (Get-Item $exe.FullName).VersionInfo.FileVersion + Write-Host "CMakeLists version : $cmakeVer" + Write-Host "expected FileVersion: $expected" + Write-Host "actual FileVersion: $actual" + if ($actual -ne $expected) { + Write-Host "::error::PE FileVersion '$actual' != expected '$expected' — the .rc template did not substitute" + exit 1 + } + Write-Host "OK: PE version resource matches the project version" + - name: Diagnose notepatra.exe imports + Qt plugin loading # Launches notepatra.exe: a Qt plugin-load stall or a modal error box would otherwise hang here forever. timeout-minutes: 5