Repository navigation
test(systest): port taskenv_test.sh to Go - #1614
Conversation
Port the env/var wiring regression contract from the POSIX-shell test/system/lib/taskenv_test.sh into a Go test (taskenv_test.go) in the systestlib module. Keeps the shell test's two-halves split: - STRUCTURAL checks parse the real Taskfile.yml with gopkg.in/yaml.v3 and assert on the parsed task/var/cmd tree, so they survive reformatting (comment edits, indentation) that the shell's awk/indent matching could not. Raw-template invariants (-o build/... outputs, forbidden date/$$/mktemp//tmp tokens) are scoped to the relevant cmd/var scalar node, not a flat file grep. - BEHAVIORAL checks drive the real `task` binary against tiny Taskfiles in t.TempDir() with a Go-controlled cmd.Env/PATH. No symlinks (repo-forbidden); the date/mktemp-off-PATH sandbox is a copied `task` binary only. Adds gopkg.in/yaml.v3 to the test/system/lib module (its first external dependency). Taskfile.yml and taskenv_test.sh are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a Go test port for the Taskfile env and HOST_EXE contract checks, plus Go module dependencies for YAML parsing. The new suite combines structural checks against ChangesTaskfile env contract port
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/system/lib/taskenv_test.go (2)
722-733: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNit:
closeparameter shadows the builtin.
extractBracketed(s, open, close string)shadows the predeclaredclose. Harmless here, butcloseTok/endreads cleaner and avoids linter noise.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/system/lib/taskenv_test.go` around lines 722 - 733, The helper extractBracketed currently uses a parameter named close, which shadows the predeclared builtin and can trigger lint noise. Rename that parameter to a clearer name like closeTok or end in extractBracketed, and update the local references within the function accordingly so the behavior stays unchanged.
609-625: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSandbox
taskcopy is unused and mis-named on Windows.
runTaskalways resolves the realtaskviataskBin(Line 512) and runs it by absolute path; the sandbox is only injected asPATHenv. So the copy written here is never executed. Separately, on Windowsexec.LookPath("task")returns a.exepath but the copy is written astask(no extension), and the comment at Lines 638-640 claims "two binaries we copy" though only one is copied. Either drop the copy (the sandbox just needs to be an otherwise-empty dir to excludedate/mktemp) or document why it must exist.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/system/lib/taskenv_test.go` around lines 609 - 625, The sandbox helper in sandboxPATH is creating a copied task binary that runTask never uses, since taskBin runs the real executable by absolute path and only the temp dir is added to PATH. Simplify sandboxPATH to just return an empty temp directory for PATH isolation, and update any nearby documentation/comments in the runTask test setup to stop claiming there are “two binaries” copied. If the copy must remain for a specific reason, ensure the name matches platform expectations, especially on Windows where exec.LookPath("task") may resolve to an .exe.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/system/lib/taskenv_test.go`:
- Around line 584-594: The filterEnv helper currently removes only exact-key
matches, so on Windows PATH entries like Path= can survive and interfere with
taskEnvPATH. Update filterEnv to use a case-insensitive key comparison when
matching against drop, and keep the behavior localized to this helper so callers
like the PATH setup in the task environment tests get a fully filtered
environment.
---
Nitpick comments:
In `@test/system/lib/taskenv_test.go`:
- Around line 722-733: The helper extractBracketed currently uses a parameter
named close, which shadows the predeclared builtin and can trigger lint noise.
Rename that parameter to a clearer name like closeTok or end in
extractBracketed, and update the local references within the function
accordingly so the behavior stays unchanged.
- Around line 609-625: The sandbox helper in sandboxPATH is creating a copied
task binary that runTask never uses, since taskBin runs the real executable by
absolute path and only the temp dir is added to PATH. Simplify sandboxPATH to
just return an empty temp directory for PATH isolation, and update any nearby
documentation/comments in the runTask test setup to stop claiming there are “two
binaries” copied. If the copy must remain for a specific reason, ensure the name
matches platform expectations, especially on Windows where exec.LookPath("task")
may resolve to an .exe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f87f2f2-b4aa-4d07-a703-935a503497a7
⛔ Files ignored due to path filters (1)
test/system/lib/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
test/system/lib/go.modtest/system/lib/taskenv_test.go
| func filterEnv(env []string, drop string) []string { | ||
| out := make([]string, 0, len(env)) | ||
| prefix := drop + "=" | ||
| for _, kv := range env { | ||
| if strings.HasPrefix(kv, prefix) { | ||
| continue | ||
| } | ||
| out = append(out, kv) | ||
| } | ||
| return out | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
sed -n '560,700p' test/system/lib/taskenv_test.goRepository: ALRubinger/aileron
Length of output: 5955
🏁 Script executed:
sed -n '560,700p' test/system/lib/taskenv_test.goRepository: ALRubinger/aileron
Length of output: 5955
🌐 Web query:
Windows environment variables case-insensitive duplicate PATH vs Path precedence CreateProcess environment block
💡 Result:
Windows environment variables are case-insensitive, meaning that PATH, Path, and path are treated as the same variable [1][2][3]. When a process is created via the Windows API, such as CreateProcess, the environment block passed to the new process is expected to be a null-terminated block of null-terminated strings [4]. While the system documentation specifies that this block should be sorted alphabetically by name (case-insensitive, Unicode order) [4], the CreateProcess function itself does not automatically sanitize or de-duplicate the environment block provided by the caller [5][6][7]. Precedence and Duplicate Handling: 1. Handling of Duplicates: If an environment block containing multiple entries for the same variable (e.g., both PATH and Path) is passed to a new process, the behavior is largely determined by how the reading process accesses the variables [7]. Many Win32 APIs and standard library functions (like GetEnvironmentVariable and getenv) that scan the environment block will typically honor the first instance encountered during a linear search [6][8][7][9]. 2. Resolution: Because the system does not enforce de-duplication during the CreateProcess call, a process may receive an environment block containing "illegal" duplicates [8]. This can cause failures in applications that parse the environment block expecting unique keys [6][8]. 3. PATH specifically: The PATH environment variable is a special case in how it is used by the system to locate executables, but it adheres to the same case-insensitive rules as all other environment variables [1][2]. When resolving a command, Windows searches directories listed in the PATH in the order they appear, but the variable itself is retrieved by the system using case-insensitive lookup [1][10]. Best Practice: It is the responsibility of the calling process (the parent) to ensure the environment block is correctly formed—specifically that it is sorted and free of duplicate keys—before invoking CreateProcess [5][6][4][9]. If you are managing environment variables in your own code, you should normalize keys to a consistent casing or de-duplicate them before passing the block to child processes to avoid compatibility issues with utilities that scan the block directly [6][8][9].
Citations:
- 1: https://stackoverflow.com/questions/48686267/windows-10-system-environment-variables-path-vs-path
- 2: https://stackoverflow.com/questions/25904872/path-and-path-environment-vars
- 3: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/getenv-wgetenv?view=msvc-170
- 4: https://learn.microsoft.com/en-us/windows/win32/procthread/changing-environment-variables
- 5: Should environment be case insensitive in Windows? klemens-morgenstern/boost-process#85
- 6: Node must de-duplicate environment variables before calling CreateProcess (and upon env.extend()) nodejs/node#35129
- 7: https://nullprogram.com/blog/2023/08/23/
- 8: Yarn on Windows breaks apps that read from the environment variables yarnpkg/yarn#8334
- 9: https://bugs.python.org/issue46862
- 10: https://stackoverflow.com/questions/5126512/how-are-windows-environment-variables-evaluated
🌐 Web query:
Windows environment variables case-insensitive duplicate PATH vs Path precedence CreateProcess environment block
💡 Result:
In Windows, environment variable names are case-insensitive [1][2]. When an application accesses an environment variable by name—for example, via the GetEnvironmentVariable API—the system typically searches the process's environment block and returns the value associated with the first matching name found, regardless of case [3]. Regarding the behavior of duplicate variable names (e.g., having both "PATH" and "Path" in the environment block) and their precedence: 1. Environment Block Structure: The environment block is a null-terminated block of null-terminated strings in the format name=value [4][5]. Windows does not natively prevent the inclusion of duplicate names that differ only by case within this block [3]. 2. Precedence and Retrieval: If an environment block contains multiple entries for the same variable name (differing only by case), the Win32 GetEnvironmentVariable function generally returns the value of the first occurrence it encounters in the block [3]. 3. PATH Special Handling: While environment variables are generally treated as case-insensitive, the PATH variable is a known exception in how the system constructs the environment for a new process [6]. When the system merges User and System environment variables, it specifically appends the User PATH to the System PATH [6][7]. This is a special behavior of the environment variable construction process, not a generic rule for all variables [6][7]. When spawning a process via CreateProcess, the parent process can provide a custom environment block [4][8]. If this block contains duplicate keys, the child process will inherit them exactly as provided [3]. Because some applications and runtime environments (such as older.NET Framework versions or specific build tools) may not handle duplicate case-insensitive keys correctly, it is a best practice for parent processes to sanitize the environment block by removing duplicates before passing it to CreateProcess [3][9].
Citations:
- 1: https://stackoverflow.com/questions/25904872/path-and-path-environment-vars
- 2: https://learn.microsoft.com/en-us/dotnet/fundamentals/runtime-libraries/system-environment-getenvironmentvariable
- 3: Environment.GetEnvironmentVariables() should tolerate duplicate case-insensitive variable names dotnet/runtime#42029
- 4: https://learn.microsoft.com/en-us/windows/win32/procthread/changing-environment-variables
- 5: https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw
- 6: https://devblogs.microsoft.com/oldnewthing/20231212-00/?p=109137
- 7: https://stackoverflow.com/questions/5126512/how-are-windows-environment-variables-evaluated
- 8: https://learn.microsoft.com/en-us/windows/win32/procthread/inheritance
- 9: win, child_process: sanitize env variables nodejs/node#35210
Make filterEnv case-insensitive for PATH on Windows
filterEnv(os.Environ(), "PATH") leaves Path=... intact on Windows, so taskEnvPATH can end up with both Path and PATH. Since Windows treats env keys case-insensitively, the inherited PATH may still take precedence and the sandboxed PATH test can remain on the host PATH. Use a case-insensitive key match when filtering env entries.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/system/lib/taskenv_test.go` around lines 584 - 594, The filterEnv helper
currently removes only exact-key matches, so on Windows PATH entries like Path=
can survive and interfere with taskEnvPATH. Update filterEnv to use a
case-insensitive key comparison when matching against drop, and keep the
behavior localized to this helper so callers like the PATH setup in the task
environment tests get a fully filtered environment.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1614 +/- ##
==========================================
- Coverage 82.37% 82.35% -0.03%
==========================================
Files 317 325 +8
Lines 31843 32399 +556
==========================================
+ Hits 26232 26683 +451
- Misses 4033 4100 +67
- Partials 1578 1616 +38
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
The committed root go.work lists ./test/system/lib, and its go.mod now requires gopkg.in/yaml.v3 (#1614). Both Go jobs that exercise the full GO_TEST_PACKAGES set (lint's `task vet:go`, and Unit Tests' `task test:go:ci:*`) now compile and run that module, but the setup-go module cache key omitted test/system/lib, leaving its yaml.v3 dependency outside the cache key. Append test/system/lib/go.sum to both cache-dependency-path blocks so the cache key stays honest for the dependency actually pulled. Operator-answered cw-review-residual decision for #1603 (issue #1610). Closes #1610 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Ports the env/var wiring regression contract from the POSIX-shell
test/system/lib/taskenv_test.shinto a Go test (test/system/lib/taskenv_test.go) in thesystestlibmodule, per #1603. The shell test is left in place (its deletion belongs to #1604);Taskfile.ymltargets are unchanged;GO_TEST_PACKAGESalready lists./test/system/lib/....The port keeps the shell test's two-halves split:
Taskfile.ymlwithgopkg.in/yaml.v3and assert on the parsed task/var/cmd tree, so they survive reformatting (comment edits, indentation changes) the shell'sawk/indent matching could not. Where an invariant is inherently about raw template text (-o build/...{{.HOST_EXE}}outputs, forbiddendate +%s/$$/mktemp//tmptokens), the assertion is scoped to the relevant cmd/var scalar node from the YAML model, not a flat file grep, so comment text can't satisfy or break it.taskbinary viaexec.Commandagainst tiny Taskfiles written tot.TempDir(), with a Go-controlledcmd.Env/PATH. Noln -ssymlinks (repo-forbidden); no externalsh/awk/sed/mktemp/date.Contracts pinned (unchanged from the shell test)
AILERON_BIN/AILERON_SYSTEST_LIB/WORKSPACEat task-levelenv:(go-task drops a command-scopedenv:).SESSION/WORKSPACEvars use only portable template funcs (nodate +%s/$$/mktemp/hardcoded/tmp).HOST_EXEsourced fromgo env GOEXE, scoped per-task (never global), appended to the three host-arch outputs and their consumers, with the forcedGOOS=linuxsandbox sibling staying extensionless.Behavioral cases: kept behavioral vs converted to structural
Per the brief's Windows-clean fallback rule, here is what each behavioral case did:
echo $AILERON_BIN, whose$expansion runs in go-task's built-in portable shell (mvdan.cc/sh), so no externalsh/probe.shis needed. Positive (task-scoped env surfaces) and negative (command-scoped env dropped) both run with a controlledcmd.Env.taskbinary (no symlinks); the pure-templateSESSION/WORKSPACEvars resolve entirely inside go-task, so the sim needs neitherdate,mktemp, nor an externalsh. This is a more faithful Windows simulation than the shell test, which had to symlinkshonly because it ransh probe.sh. A sim-validity subtest first asserts the sandbox truly lacksdate/mktemp.HOST_EXE: { sh: go env GOEXE }under forcedGOOS=windows(expectbuild/aileron.exe) and forced nativeGOOS(expectbuild/aileron<native GOEXE>) viacmd.Env, so it is deterministic regardless of host OS.No case had to be converted to structural; all three stayed behavioral and Windows-clean.
No-silent-skip
If
taskorgois not resolvable on PATH the test callst.Fatal(loud failure), matching the shell test'sexit 1. Nevert.Skip.Test plan
go test ./test/system/lib/...green on macOS (go 1.26.4, task 3.49.1), and via the fulltask test:goworkspace run.go vet ./...clean;gofmt -lclean.{{.HOST_EXE}}from launchAILERON_BIN; injectdate +%sinto aSESSION; delete aSESSIONvar) each produces the expected red, then the Taskfile restores byte-for-byte.majorfindings (presence/shape assertions forSESSION/WORKSPACE; dropped the unnecessaryshcopy from the sandbox).Taskfile.ymlandtaskenv_test.share byte-unchanged (git diffempty for both).Coverage note
This is a test-only port:
taskenv_test.gois itself the unit under test (it asserts against the realTaskfile.ymland the realtask/gobinaries). No production (.gonon-test) code is added, so there is no separate library surface to cover beyond the_test.goexercised directly bygo test.Closes #1603
🤖 Generated with Claude Code
Summary by CodeRabbit