picatz / picatz/flowstate

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

Open
#1,672 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

cli engine enhancement
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:

  1. netpolicy/netpolicy.go:666: var lookupTargetAddrs = net.DefaultResolver.LookupNetIP, mutated by netpolicy_test.go:928. This is the resolver behind checkProxiedTarget, the code that decides whether a proxied request may reach a private or metadata address. A package-level var is process-global: a future t.Parallel() in the package, or a forgotten cleanup, swaps the real resolver under every other test. A resolver field on Policy, defaulted in the constructor, is the same seam without the shared state.
  2. cmd/flow/credentials.go:56: var allowPlaintextCredential = os.Getenv("FLOWSTATE_INSECURE_PLAINTEXT_TOKEN") == "true". A security toggle read at package init cannot be exercised with t.Setenv and is global mutable state on the credential path. Reading it where it is used is equivalent and testable.
  3. engine/workflow.go:14: type ErrRunFailed struct is the only undocumented exported type in the sweep, and the Err prefix is Go's convention for sentinel values, not types; it is used with errors.As, which the name steers readers away from. RunFailedError is the convention.
  4. sort where slices fits, 23 sites in ten files that still import sort beside 198 slices.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.
  5. strings.Split ranged over once where SplitSeq fits: 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.
  6. Counted loops with an unused index where for range n fits: engine/execute.go:1718,1782, cmd/flow/mcpdebug.go:534, cmd/flow/mcp.go:891; and a pre-1.22 loop-variable capture at flowtest/run.go:1360 (go func(j job){...}(j)).
  7. 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; and cmd/flow/test.go:394 takes three adjacent bools (coverageRequired, failOnWarning, filterActive), the classic transposition the compiler cannot catch. An options struct per site.
  8. An unchecked io.Copy at plugin/sdk/sdk.go:632, where _, _ = would say the discard is deliberate, as auth/issuer.go:275,394 already 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 _test returns nothing.
  • netpolicy runs its resolver tests under t.Parallel().
  • A credentials_test.go case sets the env with t.Setenv and observes the toggle.
  • ErrRunFailed is renamed with a type alias kept for one release if any external caller exists (embed is the one to check).

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.