idioms: a process-global resolver on the SSRF path, an env toggle frozen at init on the credential path, an `Err`-prefixed error type, and 23 `sort` sites beside 198 `slices` ones
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 9
- Forks
- 0
- Avg merge
- 3h 3m
- Merged PRs (30d)
- 509
Description
Observed behavior
A sweep of the non-test code in server, engine, netpolicy, auth, plugin, flowtest, and cmd/flow at eb8172f found the high-severity categories clean: every io.ReadAll is behind a limit, every http.Server and client sets timeouts, no InsecureSkipVerify, no time.After in loops, no unbounded caches, no init() outside generated code, goroutines all have a stop channel or a wait group, and exported-doc coverage is about 97 percent. What remains is idiom-grade, and each item is cheap on its own:
netpolicy/netpolicy.go:666:var lookupTargetAddrs = net.DefaultResolver.LookupNetIP, mutated bynetpolicy_test.go:928. This is the resolver behindcheckProxiedTarget, the code that decides whether a proxied request may reach a private or metadata address. A package-level var is process-global: a futuret.Parallel()in the package, or a forgotten cleanup, swaps the real resolver under every other test. Aresolverfield onPolicy, defaulted in the constructor, is the same seam without the shared state.cmd/flow/credentials.go:56:var allowPlaintextCredential = os.Getenv("FLOWSTATE_INSECURE_PLAINTEXT_TOKEN") == "true". A security toggle read at package init cannot be exercised witht.Setenvand is global mutable state on the credential path. Reading it where it is used is equivalent and testable.engine/workflow.go:14:type ErrRunFailed structis the only undocumented exported type in the sweep, and theErrprefix is Go's convention for sentinel values, not types; it is used witherrors.As, which the name steers readers away from.RunFailedErroris the convention.sortwhereslicesfits, 23 sites in ten files that still importsortbeside 198slices.Sort*uses:flowtest/run.go:1238,1507,1733,1891,2080,flowtest/transcript.go:585,648,flowtest/coverage.go:437,486,487,554,flowtest/stub.go:309,947,1062,flowtest/schedules.go:279,cmd/flow/suggest.go:111,cmd/flow/breaking.go:245,290,308,cmd/flow/test.go:775,cmd/flow/internal/docsgen/cli.go:98,142.strings.Splitranged over once whereSplitSeqfits:plugin/binfmtmisc_linux.go:171,flowtest/anchor.go:175,flowtest/flowtesting/flowtesting.go:416,cmd/flow/plugins.go:338,cmd/flow/tasks.go:526,cmd/flow/breaking.go:677,cmd/flow/help.go:172,558,cmd/flow/internal/docsgen/tasks.go:116.- Counted loops with an unused index where
for range nfits:engine/execute.go:1718,1782,cmd/flow/mcpdebug.go:534,cmd/flow/mcp.go:891; and a pre-1.22 loop-variable capture atflowtest/run.go:1360(go func(j job){...}(j)). - Parameter lists past six, several ending in a bare bool:
engine/runtime.go:136,164(continueOnError bool),engine/execute.go:1544,1633(descend bool),engine/workflow.go:1163,flowtest/check.go:373,auth/broker.go:289; andcmd/flow/test.go:394takes three adjacent bools (coverageRequired, failOnWarning, filterActive), the classic transposition the compiler cannot catch. An options struct per site. - An unchecked
io.Copyatplugin/sdk/sdk.go:632, where_, _ =would say the discard is deliberate, asauth/issuer.go:275,394already does.
Desired outcome
Each item fixed as a move-only or mechanical change, with the two that touch security paths (1 and 2) first and each carrying the test that the old shape could not have.
Acceptance criteria
grep -rn '"sort"' pkg cmd --include='*.go' | grep -v _testreturns nothing.netpolicyruns its resolver tests undert.Parallel().- A
credentials_test.gocase sets the env witht.Setenvand observes the toggle. ErrRunFailedis renamed with a type alias kept for one release if any external caller exists (embedis the one to check).
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with netpolicy/netpolicy.go:666 and cmd/flow/credentials.go:56, then read their existing tests, including netpolicy/netpolicy_test.go:928. Work through the listed engine, flowtest, auth, plugin, and cmd/flow sites, preserving behavior and adding the requested security-path tests. Run the grep acceptance check and the affected Go tests; done means all listed idiom issues are addressed and acceptance criteria pass.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend, cli, security, testing
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 35/100