flowstate/v1: the secret-walk depth comment and its test now disagree about what can be nested past MaxStructureDepth
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 9
- Forks
- 0
- Avg merge
- 3h 3m
- Merged PRs (30d)
- 509
Description
Two prose-accuracy items from the round-3 review of #1991, both marked optional there and deferred rather than folded in, because neither changes behaviour and fixing them would have restarted the shipping gates on a comment. Recorded so they do not sit as a silent contradiction.
Problem or observed behavior
#1991 corrected secretRefs' doc comment, which claimed "the compiler admits deeper nesting than this walk inspects". It does not: flowfile's structureValue refuses a structure past MaxStructureDepth (pkg/flowstate/v1/flowfile/secret.go:319), and CheckStructureDepth refuses the same on every submit path (pkg/flowstate/v1/inputs.go:118), which pkg/flowstate/embed and both drivers reach through BindRunInputs.
The test for that same function still carries the old claim, in the present tense. pkg/flowstate/v1/secretstructure_test.go:170-172 says the compiler admits nesting deeper than the walk inspects. So the function's doc and its test now read as contradicting each other, which is worse than either being wrong alone — a reader reconciling them has no way to tell which is current.
Separately and more mildly, the replacement sentence is loose where the old one was wrong. pkg/flowstate/v1/structure.go:348 says "nothing bounds how deeply that party nested what it sent" of a plugin's outputs. Absolute depth is bounded — by the 4 MiB plugin response cap and by protobuf's default recursion limit. The operative claim, and the true one, is narrower: nothing bounds it to the walk's reach before the walk runs.
Evidence
The two statements as they stand:
pkg/flowstate/v1/structure.go:340-343 no Flowfile can express it. The compiler refuses a structure nested past
[MaxStructureDepth], and [CheckStructureDepth] refuses the same on every
submit path
pkg/flowstate/v1/secretstructure_test.go:170-172 …the compiler admits nesting deeper than the walk inspects…
Both refusal sites, read in the tree rather than taken from a review:
$ sed -n '319,324p' pkg/flowstate/v1/flowfile/secret.go
if sdepth > v1.MaxStructureDepth {
c.report(spanOfNode(n), r,
"nests a structure more than %d levels deep, …
$ sed -n '118p' pkg/flowstate/v1/inputs.go
if err := CheckStructureDepth(wf); err != nil {
That embed reaches the check rather than bypassing it: pkg/flowstate/embed/run.go:222 → v1.RunWithInputs → pkg/flowstate/v1/eval.go:1244 BindRunInputs → inputs.go:118. (An earlier draft of #1991's comment asserted the opposite; it was corrected before commit, and the commit message records that.)
The reachable path the corrected comment names: scrubPluginOutputs (pkg/flowstate/v1/plugin/task.go:530) runs inside def.Fn, and the only depth bound on a task result, CheckTaskOutputDepth, runs after it — eval.go:3026 calls def.Fn, :3057 checks depth. So at the moment the refusal is decided the depth is the peer's number, which is what makes the conservative answer load-bearing rather than pedantry.
structure.go:45-49 carries the old framing too but is explicitly past tense ("Before this was unified"), so it is correct as written and not part of this.
Desired outcome
One account of what can be nested past MaxStructureDepth, in the function's doc and in its test, derived from the refusal sites rather than restated. And the plugin-outputs sentence says what is actually unbounded.
Acceptance criteria
pkg/flowstate/v1/secretstructure_test.go's comment no longer states that the compiler admits nesting pastMaxStructureDepth, and either names the plugin-output path or points atsecretRefs' doc rather than keeping a second copy of the reasoning.structure.go:348says nothing bounds the nesting to the walk's reach before the walk runs, rather than that nothing bounds it at all.- No behaviour change and no test assertion changed:
ValueHoldsSecretRef's conservative answer past the bound stays pinned in both directions bysecretstructure_test.go:190-196.
Constraints and dependencies
Comment-only, so there is nothing to verify beyond go build, tools/citations, and the existing tests staying green. Worth doing inside whatever next change opens either file rather than on its own, which is the same disposition #495 asked for the uuid conversion and the opposite of what happened there — so if neither file is opened in a reasonable window, it is worth doing directly rather than waiting indefinitely.
pkg/flowstate/v1/refs.go:301 cites "CLAUDE.md's secretRefs shape", and no markdown in the tree contains either that spelling or the walkSecretRefs one it replaced, so that citation was already dangling before #1991 renamed the identifier. Adjacent, cheap to fix in the same pass, and not worth its own issue.
Open questions
None. Both statements have a known-true replacement; this is only about which change carries them.
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
Read pkg/flowstate/v1/structure.go around line 348 and pkg/flowstate/v1/secretstructure_test.go around lines 170-196, then check the refusal sites in flowfile/secret.go and inputs.go. Align the comments with the bounded compiler and submit paths, and narrow the plugin-output wording to the walk's reach. Run go build, tools/citations, and the existing tests; no behavior or test assertions should change.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- documentation
- Issue type
- Documentation
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 84/100