feat: standalone local-only mode + docs refresh (QUA-1525)#156
Conversation
Add a standalone mode that runs qmax-code as a local repository agent with no QualityMax account, auth, or cloud requests. Enable per-run with --local, or persist via `config set local_only true` (also honored through the QMAX_LOCAL_ONLY env var so CLI-backend MCP subprocesses inherit the boundary). Enforcement is layered, not prompt-only: - Tool catalog filters to a local allowlist (read_file, edit_file, write_file, run_command, update_plan) in both the native agent and the MCP server. - executeTool re-checks the boundary by name at call time, since tool discovery is not a security boundary for MCP clients. - cc/codex/opencode backends propagate QMAX_LOCAL_ONLY into spawned `serve --mcp` children. - Dedicated standalone system prompt; live feed, project detection/resume, and cloud session sync are disabled. Harden the local file-op boundary against symlink escape: localWorkspacePath now resolves symlinks on both cwd and the target (via the longest existing ancestor, since write_file creates new files) before the containment check, so a workspace symlink pointing outside can no longer redirect edit/write. Refresh README, ORCHESTRATION, COMMANDS, SECURITY, CONTRIBUTING, scope, changelog, and in-app /help to document standalone mode and correct stale orchestration help.
Sigilix OverviewEffort: 4/5 (large) Quality gates
Summary — latest pushIntroduces a standalone local-only mode ( Important files
Sequence diagramsequenceDiagram
participant User
participant Main as main.go
participant MCP as internal/mcp/server.go
participant Agent as internal/agent
User->>Main: --local flag / QMAX_LOCAL_ONLY=1
Main->>Main: resolveLocalOnly()
Main->>Main: Skip LoadAuth, LoadQMaxConfig
Main->>Agent: NewAgent(LocalOnly=true)
Agent->>Agent: BuildToolDefsForMode(true)
Main->>MCP: RunServer(localOnly=true)
MCP->>Agent: BuildMCPToolDefsForMode(true)
MCP->>Agent: executeTool (cloud tool name)
Agent-->>MCP: Reject: unavailable in standalone mode
Confidence: 2/5The P1 nil-panic risk in
Suggested labels:
|
sigilix
Bot
added
documentation
![sigilix[bot]](https://avatars.githubusercontent.com/in/3558711?s=60&v=4){kind=small}
| Available actions: | ||
| - read_file: Read a workspace file. Params: {"path": "relative/path"} | ||
| - run_command: Run one allowlisted local command. Params: {"command": "go test ./..."} | ||
| - edit_file: Replace exact text in a workspace file. Params: {"path": "relative/path", "old_text": "exact old text", "new_text": "replacement"} |
There was a problem hiding this comment.
Ollama local-only prompt omits update_plan tool, causing inconsistency with available tool set
The ollamaLocalToolPrompt constant lists only read_file, run_command, edit_file, and write_file as available actions, but the standalone tool catalog includes update_plan and the system prompt instructs the model to use it for multi-step work. This mismatch may cause the Ollama backend to avoid planning steps, leading to incomplete task execution. Add update_plan to the action list with its parameter description.
Example:
User: 'Refactor the auth module and add tests.'
Model (Ollama local-only): outputs a series of edit/run actions without ever calling update_plan, potentially losing track of the multi-step plan.
Suggested fix:
Add to the action list:
- update_plan: Track multi-step work. Params: {"plan": "description of steps"}Why this wasn't caught: TestStandaloneOllamaUsesLocalActionsAndExecutionGuard does not assert that update_plan appears in the instructions.
More Info
- Threat model: Users relying on Ollama in standalone mode for complex tasks may receive incomplete results because the model is not prompted to use the planning tool.
- Specific code citations: ollamaLocalToolPrompt constant (lines 49-79), buildLocalSystemPrompt (agent.go) lists update_plan, BuildToolDefsForMode(true) includes update_plan.
- Existing protections: The test TestStandaloneOllamaUsesLocalActionsAndExecutionGuard checks for the four listed tools but does not verify update_plan is present; no other guard ensures the prompt matches the catalog.
- Proposed mitigation: Add
update_planto theAvailable actionslist inollamaLocalToolPromptwith its parameter description. - Alternative mitigations considered: Removing update_plan from the native tool catalog would also resolve the inconsistency, but that would break multi-step planning for the native agent.
- Severity calibration: Score 4: under plausible multi-step requests the model may skip planning, leading to incomplete or disorganized work. Not a crash, but a functional gap.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/ollama_agent.go
Line: 63
Comment:
**Ollama local-only prompt omits `update_plan` tool, causing inconsistency with available tool set**
The `ollamaLocalToolPrompt` constant lists only `read_file`, `run_command`, `edit_file`, and `write_file` as available actions, but the standalone tool catalog includes `update_plan` and the system prompt instructs the model to use it for multi-step work. This mismatch may cause the Ollama backend to avoid planning steps, leading to incomplete task execution. Add `update_plan` to the action list with its parameter description.
Example:
User: 'Refactor the auth module and add tests.'
Model (Ollama local-only): outputs a series of edit/run actions without ever calling update_plan, potentially losing track of the multi-step plan.
Threat model:
Users relying on Ollama in standalone mode for complex tasks may receive incomplete results because the model is not prompted to use the planning tool.
Specific code citations:
ollamaLocalToolPrompt constant (lines 49-79), buildLocalSystemPrompt (agent.go) lists update_plan, BuildToolDefsForMode(true) includes update_plan.
Existing protections:
The test TestStandaloneOllamaUsesLocalActionsAndExecutionGuard checks for the four listed tools but does not verify update_plan is present; no other guard ensures the prompt matches the catalog.
Proposed mitigation:
Add `update_plan` to the `Available actions` list in `ollamaLocalToolPrompt` with its parameter description.
Alternative mitigations considered:
Removing update_plan from the native tool catalog would also resolve the inconsistency, but that would break multi-step planning for the native agent.
Severity calibration:
Score 4: under plausible multi-step requests the model may skip planning, leading to incomplete or disorganized work. Not a crash, but a functional gap.
Suggested fix shape:
Add to the action list:
- update_plan: Track multi-step work. Params: {"plan": "description of steps"}
Why this wasn't caught:
TestStandaloneOllamaUsesLocalActionsAndExecutionGuard does not assert that update_plan appears in the instructions.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Good catch — valid. ✅ Fixed in 01cf2ed.
Confirmed it's a real functional gap, not just cosmetic: in local-only mode executeOllamaAction routes through ExecuteTool, which does handle update_plan, so the model was allowed to call a tool it was never told about. 🧭
Added it to the action list with its actual param shape ({"steps":[{"title","status"}]}) rather than the guessed {"plan":"..."}, and hardened the test to assert every localOnlyToolNames entry appears in the Ollama instructions so the prompt can't silently drift from the catalog again. 🔒
There was a problem hiding this comment.
Fix verified in the branch. ollamaLocalToolPrompt at internal/agent/ollama_agent.go:68-69 now documents update_plan with its real param shape ({"steps":[{"title","status"}]}), and TestStandaloneOllamaUsesLocalActionsAndExecutionGuard at internal/agent/local_only_test.go:82-87 asserts every localOnlyToolNames entry appears in the Ollama instructions — so the prompt can't silently drift from the catalog again. No remaining gap.
The standalone Ollama action list omitted update_plan even though it is in
the local tool catalog, is executable (executeOllamaAction routes local-only
actions through ExecuteTool), and the native standalone system prompt tells
the model to use it for multi-step work. Add it with its real param shape
({"steps":[{"title","status"}]}), and assert every localOnlyToolNames entry
appears in the Ollama instructions so the prompt cannot drift from the
catalog again.
Addresses Sigilix P1 review finding on PR #156.
✅ QualityMax Pipeline
Powered by QualityMax — AI-Powered Test Automation |
| if ok && result != "" { | ||
| return result, nil | ||
| } | ||
| if a.Cfg.Context != nil && a.Cfg.Context.LocalOnly && a.Cfg.AnthropicKey == "" { |
There was a problem hiding this comment.
Ollama fallback error path in standalone mode lacks a test for the failure case
When a.Cfg.Context.LocalOnly is true and a.Cfg.AnthropicKey is empty, the Ollama fallback returns a specific error message. The existing test TestStandaloneOllamaUsesLocalActionsAndExecutionGuard only checks that cloud tools are blocked, not that the fallback error path is triggered when Ollama fails. This error path is reachable in standalone mode and should be covered to prevent regression.
Add a test case that mocks an Ollama failure in standalone mode with no Anthropic key configured, and verify the error message is returned.
More Info
- Threat model: If the Ollama backend fails in standalone mode without an Anthropic fallback, the agent should surface a clear error instead of hanging or crashing. An untested error path could regress silently.
- Specific code citations: Lines 371-378 in agent.go: the condition
if a.Cfg.Context != nil && a.Cfg.Context.LocalOnly && a.Cfg.AnthropicKey == ""returns a formatted error. - Existing protections: The test
TestStandaloneOllamaUsesLocalActionsAndExecutionGuardasserts that cloud tools are blocked, but does not simulate an Ollama failure with empty Anthropic key. - Proposed mitigation: Add a test case that mocks an Ollama failure in standalone mode with no Anthropic key configured, and verify the error message is returned.
- Alternative mitigations considered: Could rely on existing integration tests, but a unit test for this specific conditional branch is more reliable.
- Severity calibration: Score 4 because it's a reachable error path in a new feature (standalone mode) that could cause user confusion or a crash if regressed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/agent.go
Line: 375
Comment:
**Ollama fallback error path in standalone mode lacks a test for the failure case**
When `a.Cfg.Context.LocalOnly` is true and `a.Cfg.AnthropicKey` is empty, the Ollama fallback returns a specific error message. The existing test `TestStandaloneOllamaUsesLocalActionsAndExecutionGuard` only checks that cloud tools are blocked, not that the fallback error path is triggered when Ollama fails. This error path is reachable in standalone mode and should be covered to prevent regression.
Add a test case that mocks an Ollama failure in standalone mode with no Anthropic key configured, and verify the error message is returned.
Threat model:
If the Ollama backend fails in standalone mode without an Anthropic fallback, the agent should surface a clear error instead of hanging or crashing. An untested error path could regress silently.
Specific code citations:
Lines 371-378 in agent.go: the condition `if a.Cfg.Context != nil && a.Cfg.Context.LocalOnly && a.Cfg.AnthropicKey == ""` returns a formatted error.
Existing protections:
The test `TestStandaloneOllamaUsesLocalActionsAndExecutionGuard` asserts that cloud tools are blocked, but does not simulate an Ollama failure with empty Anthropic key.
Proposed mitigation:
Add a test case that mocks an Ollama failure in standalone mode with no Anthropic key configured, and verify the error message is returned.
Alternative mitigations considered:
Could rely on existing integration tests, but a unit test for this specific conditional branch is more reliable.
Severity calibration:
Score 4 because it's a reachable error path in a new feature (standalone mode) that could cause user confusion or a crash if regressed.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Fair, but this one is an integration seam, not a unit. 🧵 The error is emitted inside runStreamingLoop, which needs a live *tui.Terminal and a failing Ollama round-trip; there is no seam to drive it in isolation without a broader harness. Documented the reasoning in 7601b2b rather than adding a brittle mock. Covered the cleanly-testable sibling branches instead.
There was a problem hiding this comment.
Acknowledged — the reasoning in 7601b2b is sound. The error path at internal/agent/agent.go:373-374 (LocalOnly && AnthropicKey == "" → "ollama backend request failed in standalone mode; no Anthropic fallback is configured") lives inside runStreamingLoop, which requires a live *tui.Terminal and a real Ollama round-trip through RunOllamaAgent. There's no injected interface seam to drive that branch in isolation; any unit test would need to construct a full Agent with a wired OllamaClient whose Available() returns true and whose RunOllamaAgent returns ok=false, plus a Terminal — which is effectively an integration harness, not a unit test. The sibling branches (Cerebras at :348-350, OllamaModeChat at :381-403, and the tool-catalog/execution guards in local_only_test.go) are the cleanly testable boundaries and are already covered. Documenting the intentional gap is the right call here over a brittle mock that would be coupled to internal wiring.
| // buildLocalSystemPrompt is the standalone-mode contract. It deliberately | ||
| // names only the tools present in BuildToolDefsForMode(true) and contains no | ||
| // QualityMax project, cloud-runner, crawl, import, or hosted-script workflow. | ||
| func (a *Agent) buildLocalSystemPrompt() string { |
There was a problem hiding this comment.
New buildLocalSystemPrompt function lacks unit test coverage for its conditional branches
The function buildLocalSystemPrompt includes conditional logic based on a.Cerebras and a.Cfg.Context.GitInfo. The existing test TestStandaloneSystemPromptMatchesToolBoundary only checks for presence/absence of keywords, not the conditional branches for Cerebras image support or Git context formatting. These branches are reachable in standalone mode and should be tested to ensure they produce correct prompts.
Add unit tests that set up a.Cerebras and a.Cfg.Context.GitInfo to trigger each branch and verify the prompt output.
More Info
- Threat model: If the conditional branches produce incorrect prompts, the agent may misbehave (e.g., missing image support hint or Git context) in standalone mode.
- Specific code citations: Lines 1193-1250 in agent.go: the function
buildLocalSystemPromptincludesif a.Cerebras != nil && api.IsCerebrasGemma4Model(a.Cerebras.Model)andif a.Cfg.Context.GitInfo != nilbranches. - Existing protections: Test
TestStandaloneSystemPromptMatchesToolBoundaryasserts keyword presence but does not exercise the conditional branches. - Proposed mitigation: Add unit tests that set up
a.Cerebrasanda.Cfg.Context.GitInfoto trigger each branch and verify the prompt output. - Alternative mitigations considered: Rely on integration tests, but unit tests are more targeted and reliable for conditional logic.
- Severity calibration: Score 4 because it's a new exported function with reachable branches that affect agent behavior in standalone mode.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/agent.go
Line: 1207
Comment:
**New `buildLocalSystemPrompt` function lacks unit test coverage for its conditional branches**
The function `buildLocalSystemPrompt` includes conditional logic based on `a.Cerebras` and `a.Cfg.Context.GitInfo`. The existing test `TestStandaloneSystemPromptMatchesToolBoundary` only checks for presence/absence of keywords, not the conditional branches for Cerebras image support or Git context formatting. These branches are reachable in standalone mode and should be tested to ensure they produce correct prompts.
Add unit tests that set up `a.Cerebras` and `a.Cfg.Context.GitInfo` to trigger each branch and verify the prompt output.
Threat model:
If the conditional branches produce incorrect prompts, the agent may misbehave (e.g., missing image support hint or Git context) in standalone mode.
Specific code citations:
Lines 1193-1250 in agent.go: the function `buildLocalSystemPrompt` includes `if a.Cerebras != nil && api.IsCerebrasGemma4Model(a.Cerebras.Model)` and `if a.Cfg.Context.GitInfo != nil` branches.
Existing protections:
Test `TestStandaloneSystemPromptMatchesToolBoundary` asserts keyword presence but does not exercise the conditional branches.
Proposed mitigation:
Add unit tests that set up `a.Cerebras` and `a.Cfg.Context.GitInfo` to trigger each branch and verify the prompt output.
Alternative mitigations considered:
Rely on integration tests, but unit tests are more targeted and reliable for conditional logic.
Severity calibration:
Score 4 because it's a new exported function with reachable branches that affect agent behavior in standalone mode.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Addressed in 7601b2b. ✅ TestBuildLocalSystemPromptBranches now exercises both branches: Git-context (asserts branch/remote/Changed files: N appear, and are absent without GitInfo) and the Cerebras Gemma-4 image-support branch (asserts the image guidance appears only when a Gemma-4 model is set). 🧪
There was a problem hiding this comment.
Confirmed — TestBuildLocalSystemPromptBranches in 7601b2b exercises both conditional branches. The Git-context branch asserts branch/remote/Changed files: N appear when GitInfo is present and are absent when it's nil, and the Cerebras Gemma-4 image-support branch asserts the image guidance appears only when a Gemma-4 model is set. Withdrawing the concern.
|
|
||
| Use your native coding-agent tools for repository inspection, edits, and local tests. The qmax MCP server is intentionally restricted to local workspace file operations and allowlisted commands. Never claim a QualityMax cloud action succeeded, never ask for a QualityMax project ID, and do not direct the user to log in unless they explicitly request a cloud capability. | ||
|
|
||
| Work from repository evidence, inspect before editing, run the narrowest relevant test after a change, and report concrete results. Never expose or hardcode credentials. |
There was a problem hiding this comment.
New cliQASystemPrompt helper function lacks a dedicated unit test
The function cliQASystemPrompt decides between local and connected CLI system prompts based on sctx.LocalOnly. While its behavior is indirectly covered by integration tests, a unit test would explicitly verify the conditional switch and the exact prompt strings returned.
Add a unit test in cc_agent_test.go (or a new test file) that calls cliQASystemPrompt with both LocalOnly true/false and asserts the returned prompt matches expectations.
More Info
- Threat model: If the conditional logic is incorrect, CLI agents (CC, Codex, OpenCode) could receive the wrong system prompt in standalone vs. connected mode, leading to tool boundary confusion.
- Specific code citations: Lines 166-176 in cc_agent.go:
cliQASystemPromptreturnslocalCLISystemPromptwhensctx.LocalOnlyis true, elseconnectedPrompt. - Existing protections: Integration tests in
mcp_reconnect_test.goandopencode_config_test.gocheck environment variable propagation but not the prompt content. - Proposed mitigation: Add a unit test in
cc_agent_test.go(or a new test file) that callscliQASystemPromptwith bothLocalOnlytrue/false and asserts the returned prompt matches expectations. - Alternative mitigations considered: Rely on existing integration tests, but a unit test is simpler and more focused.
- Severity calibration: Score 3 because the function is small and the risk is bounded, but it's a new conditional in a security-sensitive boundary (tool catalog).
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/cc_agent.go
Line: 168
Comment:
**New `cliQASystemPrompt` helper function lacks a dedicated unit test**
The function `cliQASystemPrompt` decides between local and connected CLI system prompts based on `sctx.LocalOnly`. While its behavior is indirectly covered by integration tests, a unit test would explicitly verify the conditional switch and the exact prompt strings returned.
Add a unit test in `cc_agent_test.go` (or a new test file) that calls `cliQASystemPrompt` with both `LocalOnly` true/false and asserts the returned prompt matches expectations.
Threat model:
If the conditional logic is incorrect, CLI agents (CC, Codex, OpenCode) could receive the wrong system prompt in standalone vs. connected mode, leading to tool boundary confusion.
Specific code citations:
Lines 166-176 in cc_agent.go: `cliQASystemPrompt` returns `localCLISystemPrompt` when `sctx.LocalOnly` is true, else `connectedPrompt`.
Existing protections:
Integration tests in `mcp_reconnect_test.go` and `opencode_config_test.go` check environment variable propagation but not the prompt content.
Proposed mitigation:
Add a unit test in `cc_agent_test.go` (or a new test file) that calls `cliQASystemPrompt` with both `LocalOnly` true/false and asserts the returned prompt matches expectations.
Alternative mitigations considered:
Rely on existing integration tests, but a unit test is simpler and more focused.
Severity calibration:
Score 3 because the function is small and the risk is bounded, but it's a new conditional in a security-sensitive boundary (tool catalog).
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Addressed in 7601b2b. ✅ TestCLIQASystemPromptSwitchesOnLocalOnly asserts LocalOnly=true returns localCLISystemPrompt, and both nil and LocalOnly=false return the connected prompt — plus a guard that the local prompt forbids asking for a QualityMax project ID. 🔒
There was a problem hiding this comment.
Verified — TestCLIQASystemPromptSwitchesOnLocalOnly directly exercises the cliQASystemPrompt conditional, and the guard asserting the local prompt forbids requesting a QualityMax project ID is the right boundary check. Withdrawing the concern.
| - update_plan: Record or update your step-by-step plan; call FIRST for multi-step work and pass the complete ordered list each time. Params: {"steps": [{"title": "step description", "status": "pending"}]} where status is "pending", "in_progress", or "done" | ||
| - read_file: Read a workspace file. Params: {"path": "relative/path"} | ||
| - run_command: Run one allowlisted local command. Params: {"command": "go test ./..."} | ||
| - edit_file: Replace exact text in a workspace file. Params: {"path": "relative/path", "old_text": "exact old text", "new_text": "replacement"} |
There was a problem hiding this comment.
New ollamaToolInstructions method lacks a test for its conditional branch
The method ollamaToolInstructions returns different tool instructions based on a.Cfg.Context.LocalOnly. The existing test TestStandaloneOllamaUsesLocalActionsAndExecutionGuard checks that standalone instructions contain local tools, but does not test the connected mode branch. Both branches should be covered to ensure correctness.
Add a test case that sets LocalOnly false and verifies the returned instructions match the connected tool prompt.
More Info
- Threat model: If the connected mode branch returns incorrect instructions, Ollama agents might be missing tool definitions or receive cloud tools incorrectly.
- Specific code citations: Lines 62-68 in ollama_agent.go:
ollamaToolInstructionsreturnsollamaLocalToolPromptwhenLocalOnlyis true, elseollamaToolPrompt. - Existing protections: Test
TestStandaloneOllamaUsesLocalActionsAndExecutionGuardonly covers the local branch. - Proposed mitigation: Add a test case that sets
LocalOnlyfalse and verifies the returned instructions match the connected tool prompt. - Alternative mitigations considered: Rely on integration tests, but a unit test is more direct.
- Severity calibration: Score 3 because it's a simple conditional, but it's part of the tool boundary for Ollama backend.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/ollama_agent.go
Line: 64
Comment:
**New `ollamaToolInstructions` method lacks a test for its conditional branch**
The method `ollamaToolInstructions` returns different tool instructions based on `a.Cfg.Context.LocalOnly`. The existing test `TestStandaloneOllamaUsesLocalActionsAndExecutionGuard` checks that standalone instructions contain local tools, but does not test the connected mode branch. Both branches should be covered to ensure correctness.
Add a test case that sets `LocalOnly` false and verifies the returned instructions match the connected tool prompt.
Threat model:
If the connected mode branch returns incorrect instructions, Ollama agents might be missing tool definitions or receive cloud tools incorrectly.
Specific code citations:
Lines 62-68 in ollama_agent.go: `ollamaToolInstructions` returns `ollamaLocalToolPrompt` when `LocalOnly` is true, else `ollamaToolPrompt`.
Existing protections:
Test `TestStandaloneOllamaUsesLocalActionsAndExecutionGuard` only covers the local branch.
Proposed mitigation:
Add a test case that sets `LocalOnly` false and verifies the returned instructions match the connected tool prompt.
Alternative mitigations considered:
Rely on integration tests, but a unit test is more direct.
Severity calibration:
Score 3 because it's a simple conditional, but it's part of the tool boundary for Ollama backend.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Addressed in 7601b2b. ✅ TestOllamaToolInstructionsConnectedBranch covers the LocalOnly=false path: asserts it returns ollamaToolPrompt, advertises cloud actions (list_projects, start_crawl), and does not leak the standalone restriction notice. 🐈
There was a problem hiding this comment.
Confirmed — commit 7601b2b adds TestOllamaToolInstructionsConnectedBranch covering the LocalOnly=false branch of ollamaToolInstructions (internal/agent/ollama_agent.go:64-68). The method returns ollamaLocalToolPrompt when LocalOnly is true and ollamaToolPrompt otherwise. The existing TestStandaloneOllamaUsesLocalActionsAndExecutionGuard covers the standalone path; the new test closes the gap by asserting the connected path returns ollamaToolPrompt, advertises cloud actions (list_projects, start_crawl), and does not leak the standalone restriction notice. Both branches are now covered. ✅
| } | ||
| } | ||
|
|
||
| func TestStandaloneToolsListContainsOnlyWorkspaceTools(t *testing.T) { |
There was a problem hiding this comment.
Fragile test: exact tool count assertion will fail when local toolset expands
The new test TestStandaloneToolsListContainsOnlyWorkspaceTools asserts the standalone tools list contains exactly 4 tools (read_file, run_command, edit_file, write_file) and fails if len(tools) != len(want). If a new local tool is added to the allowlist (e.g., update_plan is already in the code but not in the expected set), the test will break even though the allowlist is still correct. The test should instead verify that every tool in the returned list is in the allowed set, and that no cloud tools are present, without requiring exact cardinality.
Change the test to verify that every tool in the returned list is in the allowed set, and that no cloud tools are present, without requiring exact cardinality.
Why this wasn't caught: The test asserts exact tool count and set, but does not verify that the allowlist is a superset of the expected tools; it will break if new local tools are added to the allowlist.
More Info
- Threat model: If the test fails due to a legitimate expansion of the local toolset, it could block valid changes or require unnecessary test updates, reducing confidence in the test suite.
- Specific code citations: Lines 74-95 in internal/mcp/server_test.go: the test asserts
len(tools) != len(want)wherewantis a fixed set of 4 tools. - Existing protections: The test currently ensures no cloud tools are present by checking against a fixed allowlist.
- Proposed mitigation: Change the test to verify that every tool in the returned list is in the allowed set, and that no cloud tools are present, without requiring exact cardinality.
- Alternative mitigations considered: Keep the exact count if the local toolset is intentionally fixed, but the code shows
update_planis already part of the standalone catalog. - Severity calibration: Score 3 because the test is fragile and will break on valid changes, but it does not indicate a current defect in the implementation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/mcp/server_test.go
Line: 76
Comment:
**Fragile test: exact tool count assertion will fail when local toolset expands**
The new test `TestStandaloneToolsListContainsOnlyWorkspaceTools` asserts the standalone tools list contains exactly 4 tools (`read_file`, `run_command`, `edit_file`, `write_file`) and fails if `len(tools) != len(want)`. If a new local tool is added to the allowlist (e.g., `update_plan` is already in the code but not in the expected set), the test will break even though the allowlist is still correct. The test should instead verify that every tool in the returned list is in the allowed set, and that no cloud tools are present, without requiring exact cardinality.
Change the test to verify that every tool in the returned list is in the allowed set, and that no cloud tools are present, without requiring exact cardinality.
Threat model:
If the test fails due to a legitimate expansion of the local toolset, it could block valid changes or require unnecessary test updates, reducing confidence in the test suite.
Specific code citations:
Lines 74-95 in internal/mcp/server_test.go: the test asserts `len(tools) != len(want)` where `want` is a fixed set of 4 tools.
Existing protections:
The test currently ensures no cloud tools are present by checking against a fixed allowlist.
Proposed mitigation:
Change the test to verify that every tool in the returned list is in the allowed set, and that no cloud tools are present, without requiring exact cardinality.
Alternative mitigations considered:
Keep the exact count if the local toolset is intentionally fixed, but the code shows `update_plan` is already part of the standalone catalog.
Severity calibration:
Score 3 because the test is fragile and will break on valid changes, but it does not indicate a current defect in the implementation.
Why this wasn't caught:
The test asserts exact tool count and set, but does not verify that the allowlist is a superset of the expected tools; it will break if new local tools are added to the allowlist.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Respectfully disagree — this is intentional. 👎 The exact-count assertion in assertExactToolSet is a drift detector: the whole point is to fail loudly when the standalone catalog changes so someone re-confirms the boundary is still correct. A looser assertion would silently accept a leaked cloud tool. Failing on expansion is the feature.
There was a problem hiding this comment.
Withdrawn. The exact-count assertion is a legitimate drift detector for a security boundary. The PR enforces local-only in two layers — discovery (exact tool list) and execution-time rejection (TestStandaloneToolsCallBlocksUndisclosedCloudTool) — and CONTRIBUTING.md now documents that new local tools require explicit re-confirmation. The test's want map is missing update_plan (already in localOnlyToolNames per internal/agent/tools.go:241 and the ToolCost free list at :1737), so the right fix is adding update_plan to the want map, not loosening the assertion. I'll withdraw the P3.
Learned something new
"Use exact-count assertions in drift detectors to fail loudly when boundaries change"
Undo: reply@sigilix forgetto this comment.
|
|
||
| // NewAgent creates a new LLM agent. | ||
| func NewAgent(cfg AgentConfig) *Agent { | ||
| localOnly := cfg.Context != nil && cfg.Context.LocalOnly |
There was a problem hiding this comment.
Local-only mode detection relies on SessionContext presence; ensure nil check is robust.
The agent's tool catalog selection depends on cfg.Context != nil && cfg.Context.LocalOnly. If cfg.Context is nil, the agent defaults to the full tool catalog, potentially exposing cloud tools in a local-only run. This is a hardening gap; the nil check should be explicit or the context should be guaranteed non-nil for local-only runs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/agent.go
Line: 122
Comment:
**Local-only mode detection relies on SessionContext presence; ensure nil check is robust.**
The agent's tool catalog selection depends on `cfg.Context != nil && cfg.Context.LocalOnly`. If `cfg.Context` is nil, the agent defaults to the full tool catalog, potentially exposing cloud tools in a local-only run. This is a hardening gap; the nil check should be explicit or the context should be guaranteed non-nil for local-only runs.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Respectfully disagree. 👎 The nil check is already present and complete: localOnly := cfg.Context != nil && cfg.Context.LocalOnly short-circuits before the deref, and every downstream site guards a.Cfg.Context != nil too. Nothing to harden here.
There was a problem hiding this comment.
You're right, I withdraw the finding. The nil check at internal/agent/agent.go:122 — localOnly := cfg.Context != nil && cfg.Context.LocalOnly — is correct Go short-circuit evaluation; a nil cfg.Context safely yields localOnly = false without a deref panic. The downstream sites I can see confirm the same pattern: repl.go guards ag.Cfg.Context.API and ag.Cfg.Context.ProjectID behind nil checks on api/cfg.CloudSync, and agent.go:849 passes a.Cfg.Context directly to ExecuteTool, which the PR body states performs its own execution-time rejection of non-local tools by name. There is no path where a nil cfg.Context dereference occurs, and the local-only boundary is enforced at both discovery and execution layers. No hardening needed.
| if a.sctx.LiveFeed { | ||
| env["QMAX_LIVE_FEED"] = "1" | ||
| } | ||
| if a.sctx.LocalOnly { |
There was a problem hiding this comment.
Environment variable QMAX_LOCAL_ONLY passed to MCP child but not validated in parent.
The MCP child process receives QMAX_LOCAL_ONLY=1 via env, but the parent's execution-time tool guard (ExecuteTool) relies on SessionContext.LocalOnly. If the env var is set but the context flag is false (or vice versa), the boundary could be inconsistent. This is a low-risk hardening gap; the two should be synchronized.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/cc_agent.go
Line: 238
Comment:
**Environment variable QMAX_LOCAL_ONLY passed to MCP child but not validated in parent.**
The MCP child process receives `QMAX_LOCAL_ONLY=1` via env, but the parent's execution-time tool guard (`ExecuteTool`) relies on `SessionContext.LocalOnly`. If the env var is set but the context flag is false (or vice versa), the boundary could be inconsistent. This is a low-risk hardening gap; the two should be synchronized.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Respectfully disagree. 👎 There is nothing to validate: the parent sets QMAX_LOCAL_ONLY=1 itself from the in-process sctx.LocalOnly bool — it is not untrusted input crossing a trust boundary. The child re-derives the mode and re-enforces the tool boundary at execution time regardless, so a tampered value cannot widen the catalog.
There was a problem hiding this comment.
You're right — QMAX_LOCAL_ONLY is set by the parent from its own in-process sctx.LocalOnly bool, not from untrusted input crossing a trust boundary. The child re-derives the mode from that env var and re-enforces the tool boundary at execution time in ExecuteTool, so a tampered value cannot widen the catalog. Withdrawing the concern.
| - create_pr: Create PR with tests. Params: {"repo_id": int, "project_id": int} | ||
| ` | ||
|
|
||
| const ollamaLocalToolPrompt = ` |
There was a problem hiding this comment.
Ollama local tool prompt includes 'update_plan' but standalone catalog may omit it.
The standalone Ollama tool instructions list 'update_plan' as available, but the native standalone catalog (BuildToolDefsForMode(true)) includes it. This is consistent, but the prompt should match the exact catalog to avoid confusion. No vulnerability, but a minor consistency gap.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/ollama_agent.go
Line: 49
Comment:
**Ollama local tool prompt includes 'update_plan' but standalone catalog may omit it.**
The standalone Ollama tool instructions list 'update_plan' as available, but the native standalone catalog (`BuildToolDefsForMode(true)`) includes it. This is consistent, but the prompt should match the exact catalog to avoid confusion. No vulnerability, but a minor consistency gap.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Respectfully disagree — this one is stale. 👎 It contradicts your own verification on the thread above: update_plan was added to ollamaLocalToolPrompt in 01cf2ed and TestStandaloneOllamaUsesLocalActionsAndExecutionGuard now asserts every localOnlyToolNames entry appears in the instructions. Prompt and catalog are aligned. No change needed.
There was a problem hiding this comment.
You're right, the finding is stale and should be dismissed.
In this PR, ollamaLocalToolPrompt (internal/agent/ollama_agent.go:49-63) explicitly lists update_plan as the first available action, matching the localOnlyToolNames catalog that BuildToolDefsForMode(true) returns. The prompt and catalog are aligned.
The original finding flagged a consistency gap that no longer exists: update_plan was added to ollamaLocalToolPrompt (commit 01cf2ed), and TestStandaloneOllamaUsesLocalActionsAndExecutionGuard now asserts every localOnlyToolNames entry appears in the instructions. The prompt-catalog contract is verified by test. No change needed.
Add focused unit tests for the standalone-boundary helpers Sigilix flagged: cliQASystemPrompt (local vs connected), ollamaToolInstructions connected branch, and buildLocalSystemPrompt Git-context / Cerebras-Gemma-4 image branches. The Ollama-fails-with-no-Anthropic-key path (agent.go) lives inside runStreamingLoop and is an integration seam, not unit-testable in isolation. Bump Version to 1.22.0 and finalize the CHANGELOG Unreleased section — the standalone local-only mode is a minor feature release.
| if err != nil { | ||
| return "", err | ||
| } | ||
| rel, err := filepath.Rel(cwd, absPath) |
There was a problem hiding this comment.
Missing ownership check on symlink ancestor resolution could allow workspace escape
The resolveThroughSymlinks function canonicalizes a target path by walking up ancestors until an existing directory is found, then rejoins the non-existent tail. This correctly prevents symlink escapes for non-existent files, but it does not verify that the resolved ancestor directory is still within the workspace after symlink resolution. An attacker who controls a symlink inside the workspace pointing to an ancestor directory outside the workspace (e.g., workspace/link -> /tmp/outside) could cause write_file to write a file outside the workspace if the ancestor directory exists and is resolvable. The containment check (filepath.Rel) compares the fully resolved path to the resolved workspace root, but the ancestor walk may stop at a directory that is outside the workspace yet still a valid symlink target, allowing the rejoined tail to be placed outside the workspace. The fix is to ensure each ancestor step validates containment before proceeding.
Example:
Workspace root: /home/user/workspace (realRoot)
Symlink: workspace/link -> /tmp/outside (where /tmp/outside exists)
Target path: link/foo.txt
resolveThroughSymlinks resolves /tmp/outside, rejoins foo.txt -> /tmp/outside/foo.txt
Containment check passes because /tmp/outside/foo.txt is outside realRoot? Actually `filepath.Rel` will fail because /tmp/outside is not a subdirectory of realRoot, causing an error. Wait: the check is after resolution, so it will fail. Need to verify: the ancestor walk may stop at /tmp/outside, which is outside realRoot, then rejoining foo.txt yields /tmp/outside/foo.txt. `filepath.Rel(realRoot, /tmp/outside/foo.txt)` will return an error because it's not a subdirectory, and the function returns an error. So the exploit may already be blocked. Let's re-evaluate.
Suggested fix:
func resolveThroughSymlinks(absPath, realRoot string) (string, error) {
if resolved, err := filepath.EvalSymlinks(absPath); err == nil {
return resolved, nil
}
parent := filepath.Dir(absPath)
if parent == absPath {
return absPath, nil
}
resolvedParent, err := resolveThroughSymlinks(parent, realRoot)
if err != nil {
return "", err
}
// Ensure resolvedParent is within realRoot
if rel, err := filepath.Rel(realRoot, resolvedParent); err != nil || strings.HasPrefix(rel, "..") {
return "", fmt.Errorf("symlink ancestor escapes workspace")
}
return filepath.Join(resolvedParent, filepath.Base(absPath)), nil
}More Info
- Threat model: A local-only user (or an LLM agent under their control) could craft a symlink inside the workspace pointing to an ancestor directory outside the workspace. If that ancestor directory exists,
resolveThroughSymlinkswill resolve to it, and the rejoined tail may be placed outside the workspace, bypassing the intended containment. - Specific code citations:
resolveThroughSymlinksat line 148-168 recursively walks upabsPathuntilfilepath.EvalSymlinkssucceeds, then rejoins the base name. The containment check at line 137 (filepath.Rel(realRoot, realPath)) occurs after full resolution, but the ancestor walk does not validate that each resolved parent stays withinrealRoot. - Existing protections: The
localWorkspacePathfunction resolves symlinks on both cwd and target, and checks containment viafilepath.Rel. However, the recursive ancestor resolution may stop at a directory that is outsiderealRootbut still a valid symlink target, allowing the final rejoined path to be outside the workspace. - Proposed mitigation: Modify
resolveThroughSymlinksto validate containment at each ancestor step: after resolving a parent directory, check if it is withinrealRootbefore proceeding. If the resolved parent is outside, return an error. - Alternative mitigations considered: 1. Reject any symlink that points outside the workspace at creation time (requires tracking symlink creation). 2. Use
filepath.EvalSymlinkson the entire path and reject if it resolves outside; this fails for non-existent files. The ancestor-walk with containment check is the minimal fix. - Severity calibration: Score 4 because the exploit requires a local user to create a symlink inside the workspace pointing to an existing ancestor directory outside, which is a plausible attack in a multi-user or compromised environment. It is not a remote unauthenticated exploit (P0), but it widens the attack surface beyond the intended boundary.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/tools.go
Line: 127
Comment:
**Missing ownership check on symlink ancestor resolution could allow workspace escape**
The `resolveThroughSymlinks` function canonicalizes a target path by walking up ancestors until an existing directory is found, then rejoins the non-existent tail. This correctly prevents symlink escapes for non-existent files, but it does not verify that the resolved ancestor directory is still within the workspace after symlink resolution. An attacker who controls a symlink inside the workspace pointing to an ancestor directory outside the workspace (e.g., `workspace/link -> /tmp/outside`) could cause `write_file` to write a file outside the workspace if the ancestor directory exists and is resolvable. The containment check (`filepath.Rel`) compares the fully resolved path to the resolved workspace root, but the ancestor walk may stop at a directory that is outside the workspace yet still a valid symlink target, allowing the rejoined tail to be placed outside the workspace. The fix is to ensure each ancestor step validates containment before proceeding.
Example:
Workspace root: /home/user/workspace (realRoot)
Symlink: workspace/link -> /tmp/outside (where /tmp/outside exists)
Target path: link/foo.txt
resolveThroughSymlinks resolves /tmp/outside, rejoins foo.txt -> /tmp/outside/foo.txt
Containment check passes because /tmp/outside/foo.txt is outside realRoot? Actually `filepath.Rel` will fail because /tmp/outside is not a subdirectory of realRoot, causing an error. Wait: the check is after resolution, so it will fail. Need to verify: the ancestor walk may stop at /tmp/outside, which is outside realRoot, then rejoining foo.txt yields /tmp/outside/foo.txt. `filepath.Rel(realRoot, /tmp/outside/foo.txt)` will return an error because it's not a subdirectory, and the function returns an error. So the exploit may already be blocked. Let's re-evaluate.
Threat model:
A local-only user (or an LLM agent under their control) could craft a symlink inside the workspace pointing to an ancestor directory outside the workspace. If that ancestor directory exists, `resolveThroughSymlinks` will resolve to it, and the rejoined tail may be placed outside the workspace, bypassing the intended containment.
Specific code citations:
`resolveThroughSymlinks` at line 148-168 recursively walks up `absPath` until `filepath.EvalSymlinks` succeeds, then rejoins the base name. The containment check at line 137 (`filepath.Rel(realRoot, realPath)`) occurs after full resolution, but the ancestor walk does not validate that each resolved parent stays within `realRoot`.
Existing protections:
The `localWorkspacePath` function resolves symlinks on both cwd and target, and checks containment via `filepath.Rel`. However, the recursive ancestor resolution may stop at a directory that is outside `realRoot` but still a valid symlink target, allowing the final rejoined path to be outside the workspace.
Proposed mitigation:
Modify `resolveThroughSymlinks` to validate containment at each ancestor step: after resolving a parent directory, check if it is within `realRoot` before proceeding. If the resolved parent is outside, return an error.
Alternative mitigations considered:
1. Reject any symlink that points outside the workspace at creation time (requires tracking symlink creation). 2. Use `filepath.EvalSymlinks` on the entire path and reject if it resolves outside; this fails for non-existent files. The ancestor-walk with containment check is the minimal fix.
Severity calibration:
Score 4 because the exploit requires a local user to create a symlink inside the workspace pointing to an existing ancestor directory outside, which is a plausible attack in a multi-user or compromised environment. It is not a remote unauthenticated exploit (P0), but it widens the attack surface beyond the intended boundary.
Suggested fix shape:
func resolveThroughSymlinks(absPath, realRoot string) (string, error) {
if resolved, err := filepath.EvalSymlinks(absPath); err == nil {
return resolved, nil
}
parent := filepath.Dir(absPath)
if parent == absPath {
return absPath, nil
}
resolvedParent, err := resolveThroughSymlinks(parent, realRoot)
if err != nil {
return "", err
}
// Ensure resolvedParent is within realRoot
if rel, err := filepath.Rel(realRoot, resolvedParent); err != nil || strings.HasPrefix(rel, "..") {
return "", fmt.Errorf("symlink ancestor escapes workspace")
}
return filepath.Join(resolvedParent, filepath.Base(absPath)), nil
}
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Summary
Adds a standalone local-only mode so qmax-code can run as a plain local repository agent with no QualityMax account, auth, or cloud requests — plus a full documentation refresh for v1.21.2 and the orchestration help. Closes QUA-1525.
Enable per-run with
--local, or persist withqmax-code config set local_only true. Works with Anthropic, Cerebras, Ollama, Claude Code, Codex, and OpenCode.The boundary is enforced in layers, not just the prompt
BuildToolDefsForModerestricts to a local allowlist (read_file,edit_file,write_file,run_command,update_plan) in both the native agent and the MCP server.executeToolrejects any non-local tool by name at call time, because tool discovery is not a security boundary for MCP clients.QMAX_LOCAL_ONLY=1into the spawnedserve --mcpchild so the boundary survives the process boundary.Symlink-escape hardening (added in review)
localWorkspacePathpreviously checked containment lexically (filepath.Rel), so a symlink inside the workspace pointing outside it (link -> /etc) could redirectedit_file/write_fileout of the workspace. It now resolves symlinks on both cwd and the target — via the longest existing ancestor, sincewrite_filecreates new files — before the containment check. Covered byTestWriteFileRejectsSymlinkEscape.Docs
README,
docs/ORCHESTRATION.md,docs/COMMANDS.md, SECURITY, CONTRIBUTING, OPEN_SOURCE_SCOPE, CHANGELOG, and in-app/helpupdated to document standalone mode and correct the stale orchestration/-qhelp. The earlier inaccurate "local work needs no QualityMax login" README wording is now accurate.Validation
go build ./...,go vet ./...,go test ./...— all passinternal/agent/local_only_test.go,TestWriteFileRejectsSymlinkEscapegolangci-lintnot installed in this environment — not run🤖 Generated with Claude Code