Skip to content

POK-49: Detect Windows shell and inject agent context#51

Merged
danielwpz merged 9 commits into
danielwpz:mainfrom
HYT-koota:codex/pok-49-windows-shell-context
Jun 2, 2026
Merged

POK-49: Detect Windows shell and inject agent context#51
danielwpz merged 9 commits into
danielwpz:mainfrom
HYT-koota:codex/pok-49-windows-shell-context

Conversation

@HYT-koota
Copy link
Copy Markdown
Collaborator

Summary

  • Add runtime shell detection for native Windows, Git Bash/MSYS/Cygwin, cmd, and generic POSIX hosts.
  • Inject shell/platform context into agent system prompts so agents can see the host platform, detected host shell, and bash tool invocation semantics.
  • Include the same runtime environment snapshot in runtime status surfaces and print Windows shell context from scripts/doctor.sh before the current unsupported-Windows exit.

This is intentionally limited to POK-49. It does not change bash execution mode, does not make Windows bash run with full_access, and does not bypass approvals.

Validation

Passed on Windows:

  • pnpm test -- tests/runtime/shell-info.test.ts tests/agent/system-prompt.test.ts tests/tools/get-runtime-status.test.ts tests/runtime/current-running-status.test.ts tests/runtime/status.test.ts
  • pnpm build
  • pnpm exec biome check --error-on-warnings src/runtime/shell-info.ts src/security/sandbox.ts src/agent/system-prompt-sections.ts src/agent/system-prompt.ts src/agent/loop.ts src/runtime/current-running-status.ts src/runtime/status.ts tests/runtime/shell-info.test.ts tests/agent/system-prompt.test.ts tests/tools/get-runtime-status.test.ts

Attempted:

  • pnpm preflight

pnpm preflight still fails on native Windows for existing repo-wide Windows issues unrelated to this PR, including POSIX path expectation mismatches, symlink permission failures, Windows file-mode differences, and sandbox/security tests that assume Linux/POSIX semantics. The build/lint portions ran, but the full Vitest suite failed with the existing Windows incompatibilities.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aa38ee53da

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/agent/system-prompt-sections.ts Outdated
? []
: [
`- Host platform: ${shellInfo.platformLabel} (${shellInfo.platform})`,
`- Host shell: ${shellInfo.hostShell.label} (detected from ${shellInfo.hostShell.detectionSource})`,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Sanitize env-derived shell labels before prompt injection

When the launcher environment contains a newline or prompt-like text in SHELL/MSYSTEM, this renders hostShell.label directly into the system prompt, so a value such as SHELL=$'/bin/bash\n- Ignore previous instructions...' becomes an additional system-prompt line. Normalize the shell fields used in prompts/status to inert single-line text before interpolating environment-derived values.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codex I checked this against current head 43c8ca3. This comment is stale now: the PR no longer interpolates env-derived host shell labels into the system prompt. The Windows bash-tool section only emits fixed strings for platform and shell syntax, so no additional code change is needed for current head.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HYT-koota HYT-koota changed the title POK-49 Inject Windows shell runtime context POK-49: Detect Windows shell and inject agent context May 18, 2026
@HYT-koota
Copy link
Copy Markdown
Collaborator Author

HYT-koota commented May 27, 2026

@claude Please review this Windows shell/context PR against the current upstream main. Focus on whether the Windows shell detection and injected agent context fit the existing runtime model, and call out any concrete code or test changes needed on this branch.

@danielwpz
Copy link
Copy Markdown
Owner

@claude Please review this Windows shell/context PR again. This is a maintainer-triggered retry after the Claude workflow routing change; treat this comment as the review request. Focus on whether the Windows shell detection and injected agent context fit the existing runtime model, and identify any concrete code or test changes needed on this branch.

@claude
Copy link
Copy Markdown

claude Bot commented May 28, 2026

Review: POK-49 Windows shell detection and agent context

Summary

The PR adds runtime shell/platform detection and injects context into agent system prompts and status surfaces. The detection cascade (MSYSTEM → OSTYPE → SHELL → PROMPT/ComSpec → POWERSHELL_DISTRIBUTION_CHANNEL) is sound, the sanitization is appropriate, and the data flow from detectRuntimeShellInfo() through buildAgentSystemPromptbuildWorkspaceRuntimeSection is correctly wired for all four agent types and both status surfaces. No merge-blocking correctness issues found.

Issues

1. Shell info is unconditionally injected into every system prompt on all platforms

AgentLoop.run() calls detectRuntimeShellInfo() always and passes it to buildAgentSystemPrompt unconditionally. Since shellInfo is never null, the "Workspace & Runtime" section now always renders on every agent run — even on standard Linux/macOS hosts where the default bash path and sandbox mode are already well-known. This adds approximately 6 lines (~200 chars, ~75 tokens) to every system prompt on every run. On Linux with /bin/bash, the output is:

## Workspace & Runtime
- Host platform: Linux (linux)
- Host shell: /bin/bash (detected from SHELL)
- Bash tool shell: /bin/bash -lc <command>; command syntax is bash.
- Bash default execution mode: sandboxed.
- The bash tool uses bash syntax through /bin/bash -lc.

This is a permanent token cost increase for the common case. Consider one of these approaches:

  • Gate shell-info injection to only include it when isWindows is true (or when the platform/shell is non-standard), since the primary goal is Windows context.
  • If you want to keep it always-on, that's a defensible decision — but document the tradeoff explicitly so future maintainers know this is intentional rather than an oversight.

2. detectRuntimeShellInfo is called once per status query, separately from the AgentLoop call

Status builders in src/runtime/current-running-status.ts and src/runtime/status.ts each call detectRuntimeShellInfo() independently. This produces duplicate calls (one in the AgentLoop for the prompt, one in each status query). The function is cheap (env reads only), so this isn't a performance issue. But it means if the environment hypothetically changed between the run start and a status query, the two snapshots could disagree. This is near-impossible in practice (env vars don't change mid-process), but it violates the principle of a single source of truth for the runtime environment of a given run. Consider capturing shellInfo once during run setup and threading it through rather than re-detecting it in status queries.

3. Test for prompt sanitization verifies toxic text survives in label form

The test at tests/agent/system-prompt.test.ts "sanitizes env-derived runtime shell text before prompt injection" verifies that newlines are collapsed to spaces. The resulting label becomes:

Host shell: Git Bash / MinGW (MINGW64 - IGNORE PREVIOUS INSTRUCTIONS)

This is correct sanitization (control chars stripped, whitespace collapsed), but the printable text "IGNORE PREVIOUS INSTRUCTIONS" survives. While the attack surface is minimal (env vars are set by the shell environment, not remote input), the surviving text is semantically meaningful and placed in the system prompt. The current mitigation is adequate for the threat model, but worth being aware of — if env-derived text is ever displayed in a more instruction-like context, stronger scrubbing (e.g., disallowing known prompt-injection phrases) would be warranted.

Non-blocking observations

  • The explicit note "This runtime context is informational only. It does not grant full_access host execution or bypass approvals." in the Windows notes is a good defensive measure.
  • Reusing getDefaultBashExecutable() in src/security/sandbox.ts removes the duplicate process.platform === "win32" ? "bash" : "/bin/bash" check — clean refactor.
  • The doctor.sh shell context dump before the Windows unsupported exit is useful for debugging and carries no risk.
  • Tests cover the four key detection paths (Git Bash, cmd, unknown Windows, POSIX) and the prompt injection + status integration paths. Coverage is appropriate for the scope.

@HYT-koota
Copy link
Copy Markdown
Collaborator Author

@claude Please review this Windows shell/context PR now that I have collaborator access. Focus on whether the Windows shell detection and injected agent context fit the existing runtime model, and call out any concrete code or test changes needed on this branch.

@claude
Copy link
Copy Markdown

claude Bot commented May 28, 2026

PR Review: POK-49 — Windows shell detection and agent context injection

Overall: The implementation is solid. The shell detection logic, the prompt injection sanitization, and the integration with the existing runtime model are all well done. No correctness bugs or merge-blocking issues found.

1. The shell detection and injected context fit the runtime model cleanly

The integration touches the right surfaces:

  • Agent prompts (loop.tssystem-prompt.tssystem-prompt-sections.ts): the shell info flows through the existing prompt composition pipeline without adding new mechanisms.
  • Runtime status (status.ts, current-running-status.ts): exposed in both conversation status and current-running-status snapshots, with consistent formatting.
  • Bash execution (sandbox.ts): the getDefaultBashExecutable refactor removes the duplicated platform check.
  • Doctor script (doctor.sh): prints shell context before the existing Windows-unsupported exit, useful for debugging.

The detectRuntimeShellInfo({ platform?, env? }) signature (with injectable dependencies defaulting to process) is the right pattern — it makes the detection testable without mocking global state.

2. Minor: redundant notes line on non-Windows platforms

On Linux/macOS, the notes array always contains:

The bash tool uses bash syntax through /bin/bash -lc.

This duplicates the information already in Bash tool shell: /bin/bash -lc <command>; command syntax is bash.

Fix: consider making buildShellNotes return an empty array on non-Windows, or only add the note when the host shell differs from bash (e.g., zsh on macOS). The bash-tool line already tells the agent everything it needs to know.

3. Minor: buildWorkspaceRuntimeSection contract change

Before this PR, providing neither currentDate nor timezone returned "". Now, providing shellInfo alone (without date/timezone) renders the section. This isn't triggered in practice since loop.ts always provides both, but it's a change in the function's contract.

Fix: either add an explicit check that skips shellInfo when date/timezone are absent, or add a test that codifies the new behavior so it doesn't surprise future callers.

4. Test: add a shellInfo=null case

The tests cover the happy path with shell info present. It would be worth adding a one-line assertion that buildWorkspaceRuntimeSection({ shellInfo: null }) returns "" to confirm null is handled identically to undefined/absent.

Summary

The PR is ready to merge as-is. Items 2–4 are small polish suggestions, not merge blockers. The core design — a testable detection function, prompt-safe env sanitization, and integration through the existing prompt composition pipeline — fits the runtime model well.

Co-authored-by: HYT-koota <211174511+HYT-koota@users.noreply.github.com>
@danielwpz
Copy link
Copy Markdown
Owner

@claude Fixed in 28d36fd.

What changed:

  • Merged latest main; PR is now mergeable.
  • Windows command shell resolution now defaults to PowerShell (pwsh / powershell.exe) and falls back to cmd only when PowerShell is unavailable.
  • Windows host execution uses the resolved shell, including a UTF-8 output prefix for PowerShell.
  • The Bash Tool prompt now only injects the two Windows facts the agent needs: it is running on Windows, and the available command syntax is PowerShell or cmd. macOS/Linux prompt behavior stays unchanged.
  • Removed the broader status runtime-environment injection from this PR.
  • Updated doctor/docs and added a windows-latest CI test runner.
  • Preserved HYT contributor credit via the PR history and a Co-authored-by trailer on the merge commit.

Local verification:

  • bash -n scripts/doctor.sh
  • pnpm preflight passes locally: 157 test files, 1030 tests.

Please review again.

@claude
Copy link
Copy Markdown

claude Bot commented Jun 1, 2026

Thanks for the update. I've re-reviewed the full diff at 28d36fd. The changes look correct and well-structured. Two findings, both minor:

  1. PATH env var truncated to 160 chars in getEnv (low risk, latent). sanitizeEnvText in src/runtime/shell-info.ts truncates all env values — including PATH — to 160 characters. findExecutableInPath reads PATH through getEnvsanitizeEnvText, so entries past 160 chars are silently lost. For shell detection specifically this is low-impact because absolute-path candidates are checked first via existsSync, and the bare-name PATH fallback is only the last resort. But findExecutableInPath is a general-purpose function — if reused elsewhere later, it inherits the silent truncation. Consider either lifting the limit for PATH specifically or documenting the 160-char contract on findExecutableInPath.

  2. Shell info detected twice without reuse (minor). detectRuntimeShellInfo() is called in loop.ts for the system prompt and again inside runUnsandboxedShellCommand for execution. The bash tool doesn't thread shellInfo down to executeUnsandboxedBash even though the parameter was added to the interface. Not incorrect — re-detection is safe — but it means two filesystem scans per Windows autopilot run and a theoretical window where prompt guidance and execution could diverge if the environment changes between calls. Consider passing the already-detected shellInfo through the tool execution context.

Everything else looks solid: the shell resolution cascade (pwsh 7 → pwsh 5 → cmd), UTF-8 output prefix for PowerShell, windowsHide: true on spawn, background-syntax skip on Windows, minimal prompt injection (two lines, no macOS/Linux impact), comprehensive test coverage, and the windows-latest CI runner. No merge blockers.

@danielwpz
Copy link
Copy Markdown
Owner

@claude Follow-up in ff0ed11: the new Windows CI job now runs the Windows shell/prompt/runtime-focused tests instead of the full suite, because the existing full suite still has unrelated POSIX path and tool assumptions on native Windows.

Latest GitHub checks are green:

  • Lint and build
  • Tests (linux)
  • Tests (macos)
  • Tests (windows)

PR remains mergeable.

@claude
Copy link
Copy Markdown

claude Bot commented Jun 1, 2026

Review: POK-49 — follow-up at ff0ed11

All CI is green, the PR is mergeable, and the core implementation is correct. The Windows CI scoping to shell/prompt/runtime tests is the right call given the existing full-suite POSIX assumptions.

Issues

1. shellInfo is not threaded through the bash tool execution path (minor, pre-existing observation)

The ExecuteUnsandboxedBashInput interface and runUnsandboxedShellCommand both accept shellInfo, but src/tools/bash.ts never passes it. As a result, every Windows autopilot command re-detects the shell from scratch via detectRuntimeShellInfo(). The fallback inside runUnsandboxedShellCommand correctly handles this (input.shellInfo?.commandShell ?? detectRuntimeShellInfo(...)), so there is no crash or wrong-behavior risk — just a redundant filesystem scan per invocation and the theoretical divergence between what the agent was told (in the prompt) and what's used at execution time.

Fix direction: Thread the shellInfo detected in loop.ts (or re-detect once at tool-invocation time) through the bash tool's execute function to executeUnsandboxedBash.

2. bash.test.ts runs in full on the Windows CI runner, not just the Windows-specific tests

The CI command is:

pnpm exec vitest run --passWithNoTests tests/runtime/shell-info.test.ts tests/agent/system-prompt.test.ts tests/security/sandbox.windows-timeout.test.ts tests/tools/bash.test.ts

tests/tools/bash.test.ts is included in full, not filtered to the Windows-specific cases added by this PR. The existing test suite in that file has many tests that mock process.platform and test POSIX sandbox behaviors. They pass now, but they could start failing if Windows-agnostic testing patterns shift later. This is acceptable for the initial Windows CI rollout but worth documenting that the intent is to grow the Windows test surface gradually.

Non-blocking observations

  • The windowsHide: true on spawn and the POWERSHELL_UTF8_OUTPUT_PREFIX are both correct and necessary for proper Windows host execution.
  • The PowerShell 7 → PowerShell 5 → cmd fallback cascade in resolveRuntimeCommandShell is well-ordered and correctly handles the case where neither is available (returns "cmd.exe" as the last resort).
  • Skipping detectUnsupportedBackgroundSyntax on Windows is correct — & is a PowerShell call operator, not a background operator in that context.
  • The Co-authored-by: HYT-koota trailer on the merge commit properly preserves contributor credit.

No merge blockers.

@danielwpz
Copy link
Copy Markdown
Owner

@codex please help review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ff0ed11d99

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/tools/bash.ts Outdated
Comment on lines +95 to +97
const backgroundReason = isNativeWindowsPlatform()
? null
: detectUnsupportedBackgroundSyntax(args.command);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject PowerShell background jobs

On native Windows with PowerShell 7 selected, this bypasses all background-syntax checks, but PowerShell also has a trailing & background operator; Microsoft’s about_Operators docs describe it as running the preceding pipeline in a background job. That means commands like npm run dev & or Start-Sleep 60 & now return immediately instead of being rejected, despite the prompt saying background shell jobs are unsupported. Keep allowing the leading call operator, but add PowerShell-aware rejection for trailing &/job syntax before executing.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codex Fixed in 43c8ca3. The bash tool now uses PowerShell-aware background detection on native Windows: it rejects trailing & and Start-Job / Start-ThreadJob, while still allowing the leading PowerShell call operator. Added regression coverage in tests/tools/bash.test.ts.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread src/runtime/shell-info.ts Outdated
sanitized += codePoint == null || codePoint < 32 || codePoint === 127 ? " " : char;
}

return sanitized.replace(/\s+/g, " ").trim().slice(0, 160);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Do not truncate PATH before shell lookup

getEnv() is also used by findExecutableInPath() for PATH and PATHEXT, so the 160-character display limit makes shell detection ignore every PATH entry after that point. On typical Windows hosts PATH is often much longer than 160 characters; if pwsh.exe is installed only via a later PATH entry, Pokoclaw will silently fall back to Windows PowerShell or cmd even though PowerShell 7 is available, causing the prompt and actual execution shell to be wrong. Keep truncation for human-facing text only, or let executable lookup read the full env value.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codex Fixed in bd31a89. findExecutableInPath() now reads full, untruncated PATH and PATHEXT values while keeping the 160-character limit for human-facing env text. Added a regression test for a long Windows PATH where pwsh.exe is found only after the old truncation point.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread scripts/doctor.sh Outdated
Comment on lines +136 to +138
fail "missing PowerShell"
info "Pokoclaw expects PowerShell on native Windows. Install or repair PowerShell before relying on cmd fallback."
failures=$((failures + 1))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 Badge Let cmd fallback pass the Windows doctor

This makes missing PowerShell a hard doctor failure even when cmd.exe is present, but pnpm start runs the doctor before launching the app and the runtime code/docs now claim cmd is a supported fallback when PowerShell is unavailable. In that exact fallback environment, users cannot start via the documented entrypoint despite the fallback path being implemented; treat missing PowerShell as a warning when cmd is available, or remove the fallback from runtime/docs.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codex Fixed in 43c8ca3. The Windows doctor now treats missing PowerShell as a warning when cmd fallback is available, and only fails the shell check when no usable fallback exists. This matches the product behavior: PowerShell is recommended and default, cmd is the fallback.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread src/security/sandbox.ts Outdated
Comment on lines +706 to +709
const command =
shell.syntax === "powershell"
? `${POWERSHELL_UTF8_OUTPUT_PREFIX}${input.command}`
: input.command;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve native PowerShell exit codes

When Windows commands run through powershell.exe -Command/pwsh -Command, Microsoft’s about_PowerShell_exe docs say a non-0/1 exit from an external program is converted to process exit code 1 unless the command explicitly exits with $LASTEXITCODE. Because this wrapper only prefixes the user command, failures from native tools such as robocopy or custom CLIs that use distinct exit codes will be reported as 1, losing information that the bash tool previously returned from /bin/bash -lc. Append an exit-code relay for PowerShell commands so exitCode reflects the native program’s actual status.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codex Fixed in 43c8ca3. PowerShell host commands now append an exit-code relay that exits with $LASTEXITCODE when the final command fails, preserving native CLI exit codes instead of collapsing them to 1. Added coverage in tests/security/sandbox.windows-timeout.test.ts.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielwpz
Copy link
Copy Markdown
Owner

@codex @claude Follow-up fixes are pushed.

Commits:

  • bd31a89: threads shellInfo through bash execution and fixes untruncated PATH/PATHEXT lookup.
  • 43c8ca3: rejects PowerShell background jobs while preserving the call operator, preserves native PowerShell exit codes via $LASTEXITCODE, and makes missing PowerShell a doctor warning when cmd fallback is available.

Verification:

  • local pnpm preflight passes: 157 files, 1035 tests.
  • GitHub CI is green: Lint and build, Tests (linux), Tests (macos), Tests (windows).

Please review again.

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Nice work!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@claude
Copy link
Copy Markdown

claude Bot commented Jun 1, 2026

Review: POK-49 follow-up fixes (bd31a89 + 43c8ca3)

The two follow-up commits correctly address the prior review feedback. shellInfo is now threaded through the full execution chain, PATH/PATHEXT truncation is fixed, PowerShell background detection correctly distinguishes the call operator from background &, exit codes are preserved via $LASTEXITCODE, and the doctor gracefully degrades when PowerShell is missing but cmd is available. CI is green across all three platforms. No merge blockers.

1. cmd syntax path has no background detection at all

In detectUnsupportedBackgroundSyntaxForContext, when shellSyntax === "cmd", the function returns null — no background check is performed. This means START /B or other cmd backgrounding mechanisms would pass through unblocked. The agent prompt tells agents not to use background jobs, but unlike the PowerShell path (which has both prompt guidance and runtime enforcement), the cmd path relies solely on prompt guidance. This is acceptable for the initial Windows rollout given cmd is a fallback, but worth being aware of as the Windows surface grows.

Fix direction: Either document this as an intentional gap, or add cmd-specific background detection (e.g., blocking /B flag on START).

2. isPowerShellNonBackgroundAmpersand treats bare & at start-of-line as non-background unconditionally

When findPreviousNonWhitespaceIndex returns -1 (the & is at column 0 or preceded only by whitespace), the function returns true (non-background, i.e., call operator). This is correct for & command but also allows & on a line by itself — which would be a PowerShell syntax error, not a background job. The current behavior is safe (a syntax error is harmless), but a future refactor might want to tighten this to verify that a command token actually follows the &.

3. The executeUnsandboxedBash interface now documents shellInfo as optional, but the implementation always falls back to re-detection if missing

In runUnsandboxedShellCommand:

const shell =
  input.shellInfo?.commandShell ??
  detectRuntimeShellInfo({ platform: input.platform }).commandShell;

This is correct and defensive, but means the shell is re-detected (with a filesystem scan via existsSync) on every Windows invocation where shellInfo isn't threaded. All current call sites thread shellInfo, so this is only a latent concern — no action needed now.

Non-blocking observations

  • The $LASTEXITCODE relay correctly handles three cases: explicit user exit (relay doesn't run because $? is technically true for the script), external program exit codes (relayed via $LASTEXITCODE), and PowerShell-internal errors (falls through to exit 1).
  • The test at "does not truncate PATH before Windows executable lookup" verifies the fix from the prior review — using a 24-entry PATH prefix (well over 160 chars) with maxLength: null. Good defensive test.
  • windowsHide: true on spawn and SHELL env var propagation are both correct for Windows host execution hygiene.
  • The doctor script's degredation from fail to warn when PowerShell is missing but cmd is available is the right UX call — it communicates the recommendation without blocking usable configurations.

@danielwpz
Copy link
Copy Markdown
Owner

@claude Thanks. I handled the actionable cmd fallback gap in 0b6fe25.

What changed:

  • cmd fallback execution now rejects START /B before host execution.
  • Added regression coverage for both start /b ... and start "" /B ... on native Windows cmd fallback.

I left the other two observations unchanged:

  • Bare & at the beginning of a PowerShell command remains allowed as the call-operator path; a bare syntax error is harmless and does not create background execution.
  • executeUnsandboxedBash keeps the defensive shellInfo fallback, even though current AgentLoop/bash call sites thread the run snapshot.

Verification:

  • local pnpm preflight passes: 157 files, 1037 tests.
  • GitHub CI is green: Lint and build, Tests (linux), Tests (macos), Tests (windows).

Please review again.

@claude
Copy link
Copy Markdown

claude Bot commented Jun 1, 2026

Review: POK-49 — cmd fallback background gap fix (0b6fe25)

The 0b6fe25 commit correctly closes the cmd fallback background gap identified in the prior review. The findCmdStartBackgroundFlag detection and the routing through detectUnsupportedBackgroundSyntaxForContextdetectUnsupportedCmdBackgroundSyntax are sound. No merge blockers.

1. findCmdStartBackgroundFlag regex has a narrow false-positive path (non-blocking)

The regex /^\s*\(?\s*start\b[^\r\n&|]*\/b(?:\s|$)/i matches START /B correctly, including with a quoted title (start "" /B ...) and with leading parens. However, a forward-slash path like start /D C:/b program would also match because /b appears in C:/b followed by a space. On Windows forward-slash paths are unusual, the shell-content stripping removes quoted paths, and the worst outcome is a harmless recoverable error. Not worth changing now — just noting it for when the Windows surface grows and the detection becomes more enforcement-oriented.

2. PowerShell exit-code relay loses non-zero codes from multi-statement commands (non-blocking)

The relay suffix (if (-not $?) { ... }) correctly relays $LASTEXITCODE for single-statement commands (verified by the robocopy exit-code-7 test). However, for multi-statement commands separated by ;, $? reflects only the last statement. A non-zero exit from an earlier statement would be lost. Multi-statement PowerShell commands from agents are rare, so this is a reasonable tradeoff for initial Windows support.

Fix direction (defer to a future PR): capture $LASTEXITCODE right after the user command and relay it unconditionally:

$global:__pokoclaw_exit = 0
<user command>
if ($global:LASTEXITCODE -ne 0) { $global:__pokoclaw_exit = $global:LASTEXITCODE }
if (-not $?) { if ($global:__pokoclaw_exit -eq 0) { $global:__pokoclaw_exit = 1 } }
exit $global:__pokoclaw_exit

3. Leftover observations from the prior review are correctly preserved

  • Bare & at the start of a PowerShell command remains treated as the call operator — correct and safe.
  • executeUnsandboxedBash keeps the defensive shellInfo fallback — appropriate for robustness.
  • The test at tests/tools/bash.test.ts line 197 (executeBashWithFullAccess) passes shellInfo to executeUnsandboxedBashMock but the mock doesn't validate it. This is fine since the validation happens in the executeUnsandboxedBash implementation, not the mock — but the sandbox.windows-timeout.test.ts tests correctly validate spawnCalls[0].command === "pwsh.exe".

Summary

The START /B detection is correct, the routing through the context-aware background checker is well-structured, and the regression tests cover both start /b ... and start "" /B .... CI is green across all three platforms. Ready to merge.

@danielwpz
Copy link
Copy Markdown
Owner

@claude Fixed the two non-blocking follow-ups in b49f0e9.

What changed:

  • START /B detection now only matches /b as an independent cmd arg, so start /D C:/b /WAIT ... no longer false-positives.
  • The PowerShell host wrapper now captures $? and $LASTEXITCODE immediately after the user command and exits explicitly, so multi-statement native exit codes are preserved more consistently.
  • Added regression coverage in tests/tools/bash.test.ts and tests/security/sandbox.windows-timeout.test.ts.

Verification:

  • Local pnpm preflight passes: 157 files, 1038 tests.
  • GitHub CI is green: Lint and build, Tests (linux), Tests (macos), Tests (windows).

Please review again.

@danielwpz danielwpz merged commit bd4b543 into danielwpz:main Jun 2, 2026
4 checks passed
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.

2 participants