Skip to content

Fix shell permission parsing and test-created debug artifacts#2536

Open
LaZzyMan wants to merge 3 commits intoQwenLM:mainfrom
LaZzyMan:fix/test-artifacts-and-shell-permissions
Open

Fix shell permission parsing and test-created debug artifacts#2536
LaZzyMan wants to merge 3 commits intoQwenLM:mainfrom
LaZzyMan:fix/test-artifacts-and-shell-permissions

Conversation

@LaZzyMan
Copy link
Collaborator

TLDR

This PR fixes three related test and shell-permission issues:

  1. It fixes shell command splitting so redirections like 2>&1 and >| are not incorrectly treated as standalone commands.
  2. It updates the ACP connection test to provide a live child-process mock so the structured prompt-block test exercises sendPrompt() correctly.
  3. It disables debug log file output in CLI tests by default, preventing test:ci from leaving behind runtime debug artifacts such as debug/latest and per-session .txt files.

Dive Deeper

The shell permission confirmation flow was incorrectly splitting commands like npm run build 2>&1 | head -100, which caused bogus permission suggestions such as Bash(1) and incorrect confirmation labels. This happened because the shell command splitter treated redirection operators as command separators. The fix makes the splitter aware of file-descriptor and redirection syntax, while preserving normal handling for &&, ||, |, ;, and newlines.

To guard against regressions, this PR adds focused tests covering:

  • file descriptor redirection in command parsing
  • >| redirection handling
  • shell confirmation details for compound commands containing redirections

This PR also fixes a failing VS Code companion test. The structured ACP prompt-block test only mocked sdkConnection, but sendPrompt() first validates connectivity through ensureConnection(), which also requires a live child. The test now provides that mock explicitly.

Finally, this PR prevents CLI test runs from creating runtime debug artifacts. Some CLI config tests intentionally switch the runtime output directory, and because CLI tests did not disable debug log files by default, later config/session initialization could create debug/latest symlinks and session log files in those temporary runtime locations. The CLI test setup now matches the core test setup by defaulting QWEN_DEBUG_LOG_FILE=0 during tests.

Reviewer Test Plan

  1. Run the targeted test suites:

    • packages/vscode-ide-companion/src/services/acpConnection.test.ts
    • packages/cli/src/config/config.test.ts
    • packages/core/src/utils/shell-utils.test.ts
    • packages/core/src/tools/shell.test.ts
  2. Verify shell parsing behavior:

    • Confirm npm run build 2>&1 | head -100 no longer produces an extra command/rule like 1 or Bash(1).
    • Confirm echo hello >| out.txt is treated as a single command.
  3. Verify the ACP test behavior:

    • Confirm the structured prompt-block test now reaches prompt() and no longer fails with Not connected to ACP agent.
  4. Verify CLI test artifact cleanup:

    • Run the CLI config tests and confirm they do not recreate runtime debug artifacts such as:
      • custom/runtime/debug/latest
      • from-env/debug/latest
      • first/runtime/debug/latest
      • .qwen/debug/latest
      • per-session debug .txt files

Testing Matrix

🍏 🪟 🐧
npm run ✅*
npx
Docker
Podman - -
Seatbelt - -

Targeted Vitest suites only; full CI matrix was not run.

Linked issues / bugs

None.

@github-actions
Copy link
Contributor

📋 Review Summary

This PR addresses three related test and shell-permission issues: fixing shell command splitting for redirections, updating the ACP connection test for proper connectivity mocking, and disabling debug log file output in CLI tests. The changes are well-targeted, include appropriate test coverage, and follow existing code patterns.

🔍 General Feedback

  • The PR is well-structured with focused changes across 5 files
  • Test coverage is comprehensive, including edge cases for shell parsing
  • The shell-utils fix correctly handles file descriptor redirections while preserving normal operator handling
  • Code style and patterns are consistent with the existing codebase
  • Good use of helper functions (previousNonWhitespaceChar) to improve readability

🎯 Specific Feedback

🟡 High

  • File: packages/core/src/utils/shell-utils.ts:155-175 - The new logic for handling & and | characters checks the previous non-whitespace character to determine if it's a redirection operator. However, there's a potential edge case: if the command starts with & or | (unlikely but possible), previousNonWhitespaceChar(i) would return undefined, and the current logic would treat it as a command separator. Consider adding an explicit check: if (!prevChar || prevChar === '>' || prevChar === '<') for clarity, though the current behavior is technically correct.

🟢 Medium

  • File: packages/core/src/utils/shell-utils.ts:155-175 - The condition (char === '|' && (nextChar === '|' || nextChar === '&')) on line 155 handles || and |&, but the subsequent else if (char === '|') block (lines 167-173) will also check for >|. This is correct, but consider adding a comment to clarify that |& (bash stderr+stdout pipe) is handled by the first condition while >| (force overwrite) is handled by the second.

  • File: packages/core/src/utils/shell-utils.test.ts:443-454 - The new tests are good but could benefit from one additional edge case: testing a command with multiple redirections like cmd1 > file1 2>&1 | cmd2 to ensure the full chain is parsed correctly.

🔵 Low

  • File: packages/cli/test-setup.ts:13-16 - Consider adding a comment explaining why QWEN_DEBUG_LOG_FILE=0 is the default for CLI tests (to match core test behavior and prevent artifact creation). The PR description explains this well, but a brief inline comment would help future maintainers.

  • File: packages/vscode-ide-companion/src/services/acpConnection.test.ts:96 - The mock child object { killed: false, exitCode: null } could be extracted to a shared constant or helper function if this pattern is used elsewhere in the test file, reducing duplication.

✅ Highlights

  • Excellent test coverage for shell parsing edge cases including 2>&1 and >| redirections
  • The previousNonWhitespaceChar helper function is a clean solution for context-aware character parsing
  • The ACP test fix correctly identifies the root cause (missing child mock for ensureConnection())
  • The CLI test setup change prevents test artifact pollution, improving CI reliability
  • The PR description is thorough with a clear testing matrix and reviewer test plan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant