picatz / picatz/flowstate

docs, engine: three comments claim more than the code supports — pinning's coverage, a redaction cost bound, and a fast path's precondition

Open Beginner friendly
#1,969 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

documentation
Dominant language
Go
Stars
9
Forks
0
Avg merge
3h 3m
Merged PRs (30d)
509

Description

Observed behavior

Three prose claims are stronger than what the code under them does. None is a correctness defect; each misprices the thing it describes for the next reader who acts on it. Collected because they surfaced together in one review round of #1962 and each is a sentence, not a change.

1. docs/ARCHITECTURE.md, "Expression evaluation placement" — pinning's coverage

The bullet names the right class — an evolving evaluator inside the replay path — then says the exposure "is held down by pinning the interpreter per run", and that the thing pinning does not cover "is the Continue-As-New seam".

Pinning covers vocabulary: CurrentProfile/OriginalProfile and profiles freeze library membership per recorded spec, and stringsExtensionVersion/listsExtensionVersion pin extension versions at build. It does not cover pricing. evaluationCostEstimator is a package-level singleton installed unconditionally by Limits.programOptions() with no profile or version input, so a change to what an expression costs reaches a replaying history exactly as it reaches a fresh one — a run can cross its slice budget at a different node than its history recorded, or be refused at DefaultCostLimit for an expression that history admitted.

That is not hypothetical: #1962 re-priced any call returning a concrete list, and says so in its own risk section and at workflowSliceCostChange's doc comment. The failure this bullet enables is an engineer sizing a future estimator change, reading "held down by pinning", checking that their change touches no Continue-As-New seam, and shipping.

2. pkg/flowstate/v1/sensitivevalues.go, markMatches — "disjoint by construction"

The comment says the spans written are disjoint by construction, so the marking costs the length of the union. The start < pendingStart arm can move pendingStart back before an already-marked span's end, so a later mark() re-covers marked bytes. Concretely, matches [0,10), [20,21), [5,30) mark [0,10) and then [5,30).

Correctness is unaffected — every match is always inside the pending span, marking is idempotent, and the hull is only ever taken over spans the current match contains, so nothing is missed and nothing outside the union is marked. Only the cost claim is wrong: the worst case is O(n·L) in the longest sensitive value rather than O(n). Not attacker-reachable, since sensitive values are operator-supplied.

3. pkg/flowstate/v1/flowfile/lsp/outline.go, yamlStringScalar — the fast path's precondition

The fast path is exactly equivalent given its one caller's scanner, which delivers a properly closed scalar with no lone interior quote. Read standalone the doc claims more: yamlStringScalar( + "" + "a"b" + "" + ) returns a"b, true where the parser returned "", false. Fine with one caller; a trap for a second.

Desired outcome

Each sentence says what its code actually guarantees. Specifically: the architecture bullet distinguishes vocabulary from price and points at workflowSliceCostChange; the redaction comment either softens the cost claim or drops it; and the outline fast path names the caller guarantee it depends on.

Acceptance criteria

  • A reader sizing an estimator change learns from docs/ARCHITECTURE.md that pinning does not cover pricing, without having to read celenv.go.
  • No claim in the three places asserts a bound or an equivalence the code does not hold.

Constraints

Documentation only; no behavior change is wanted here. If the architecture bullet's trade is revisited on the strength of the corrected text, that is a separate change with its own issue.

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 the three locations named in the issue: the “Expression evaluation placement” section in docs/ARCHITECTURE.md, markMatches in pkg/flowstate/v1/sensitivevalues.go, and yamlStringScalar in pkg/flowstate/v1/flowfile/lsp/outline.go. Review the referenced workflowSliceCostChange comment and the fast path’s caller context. Done means each sentence states only the guarantee supported by its code, with no behavior changes.

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
88/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.