fix(execd): parse dash/ash export -p and follow-up shell-fallback cleanups#1367
Open
Pangjiping wants to merge 1 commit into
Open
fix(execd): parse dash/ash export -p and follow-up shell-fallback cleanups#1367Pangjiping wants to merge 1 commit into
Pangjiping wants to merge 1 commit into
Conversation
github-actions
Bot
added
component/execd
size/M
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3beb82408b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Pangjiping
force-pushed
the
fix/execd-shell-fallback-followups
branch
from
July 22, 2026 02:47
3beb824 to
e7ba57c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7ba57c9d9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Pangjiping
force-pushed
the
fix/execd-shell-fallback-followups
branch
from
July 22, 2026 03:02
e7ba57c to
5ef302a
Compare
github-actions
Bot
added
size/L
Follow-ups from PR #1361 review. Parser fix (main change) PR #1361 added a POSIX single-quoted branch to parseExportLine that only handled the '\'' escape emitted by bash / zsh / BSD sh and assumed the value was wrapped in a matching pair of '...'. Neither assumption holds under dash / BusyBox ash — the default sh on Alpine and Debian slim images this fallback exists for: - dash writes embedded single quotes as '"'"' rather than '\''. - dash does not wrap the whole value in a single pair of quotes when the value starts or ends with a single quote. For example trailing' becomes 'trailing'"'"' — not '-terminated. shellEscape writes the '"'"' form as well, so any session variable containing a single quote was silently corrupted on the next /session run whenever bash was unavailable. Replace the strip-and-replace approach with unquoteShellWord, a proper POSIX shell-word unquoter that walks the raw string and consumes '...', "...", and \\c segments in order. Tests added to lock in the wire format: - TestParseExportLine_BashAndShFormats table rows covering trailing, leading, both-side, single-', and multi-quote runs. - TestParseExportLine_DashFormatRoundTrip driven by dashExportEscape, a helper that mirrors /bin/dash 0.5.x export -p output (verified against real dash). - TestShellEscapeParseExportLine_RoundTrip expanded with more edges. - TestBashSession_FallsBackToSh_PersistsSingleQuotedValue exercises the full write-then-read cycle under sh. Reverse-verified: reverting only bash_session.go while keeping the new tests reproduces the corruption, confirming the tests actually gate the fix. Shell-launch consolidation The five shell-launch sites in pkg/runtime each hand-rolled the same 'preferred shell + optional --noprofile --norc' logic, with drift in flag ordering. Extract shellCommand() so bash-session, PTY, and isolated-session launches share the same code path. The two Controller.runCommand* sites keep their inline getShell + explicit '-c' invocation because --noprofile/--norc are no-ops for one-shot 'bash -c'. Cache getShell() with sync.Once — execd's PATH is fixed at startup, so re-running exec.LookPath on every request is pure overhead. Expose resetShellCacheForTest for tests that mutate PATH, wired into useShOnlyPath. Test helper consolidation 25+ tests inlined the same 'exec.LookPath("bash"); err != nil { t.Skip }' guard, with two divergent skip messages. Extract requireBash(t) into both pkg/runtime and pkg/web/controller. The helper lives in a testhelpers_test.go without a !windows build tag so command_test.go — which is compiled on Windows — can reference it; resetShellCacheForTest and useShOnlyPath remain in shell_fallback_test.go (!windows) where their symlink-based setup applies.
Pangjiping
force-pushed
the
fix/execd-shell-fallback-followups
branch
from
July 22, 2026 03:15
5ef302a to
7295518
Compare
Collaborator
Author
|
@bcho ready for review. 😊 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72955188d4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
bcho
approved these changes
Jul 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-ups from PR #1361 review, split into three logically independent commits:
fix(execd): parse sh export -p output using dash/ash quote conventionPR fix(execd): fall back to sh in bash and isolated sessions #1361 added a POSIX single-quoted branch to
parseExportLinethat only handled the'\''escape emitted by bash / zsh / BSD sh, but dash and BusyBox ash — the defaultshon Alpine / Debian slim images this fallback exists for — emit embedded single quotes as'"'"'instead.shellEscapealso writes the'"'"'form, so on a bash-less image any session variable containing a single quote was silently corrupted on the next/sessionrun.Accepted both escape forms in the parser and locked the invariant with:
TestParseExportLine_BashAndShFormatsTestShellEscapeParseExportLine_RoundTripfuzz-style round-tripTestBashSession_FallsBackToSh_PersistsSingleQuotedValueend-to-endrefactor(execd): unify shell launch via shellCommand helper and cache getShellThe five shell-launch sites in
pkg/runtimewere each hand-rolling the same "preferred shell + optional--noprofile --norc" logic, with drift in flag ordering. ExtractshellCommand()so bash-session, PTY, and isolated-session launches share the same code path. The twoController.runCommand*sites keep their inlinegetShell+ explicit-cinvocation because--noprofile/--norcare no-ops for one-shotbash -c.Cache
getShell()withsync.Once— execd's PATH is fixed at startup, so re-runningexec.LookPathon every request is pure overhead.resetShellCacheForTestis exposed for tests that mutate PATH (wired intouseShOnlyPath).test(execd): replace inline bash lookups with requireBash helper25+ tests inlined the same
exec.LookPath("bash"); err != nil { t.Skip }guard with two divergent skip messages. ExtractrequireBash(t)(in bothpkg/runtimeandpkg/web/controller) so future bash-dependent tests can opt in with one line and a consistent reason.Each commit builds and passes tests standalone (bisect-safe).
Testing
Passed on darwin/arm64:
cd components/execd && go build ./...cd components/execd && go vet ./pkg/runtime ./pkg/web/controllercd components/execd && go test ./pkg/runtime ./pkg/web/controller -count=1cd components/execd && go test ./pkg/runtime -run 'TestParseExportLine|TestShellEscape|TestBashSession_FallsBackToSh|TestPTYSession_FallsBackToSh|TestIsolatedSession_FallsBackToSh' -count=10parseExportLinehunk while keeping the new tests reproduces the failure on the dash/ash quote-escape cases, confirming the tests actually gate the fix.Breaking Changes
No public API, config, CRD, or CLI surface changes. Internal refactor only.
Checklist