fix: pine-cpp OperatorInput skip/template gate (#174) + pine-java snapshotKeys type-tag dispatch (#177) - #178
Merged
Merged
Conversation
pine-go and pine-java exclude skip control fields and common_input_template source fields from OperatorInput.common at buildInput time — the operator sees nil for them. pine-cpp's OperatorInput is a lazy proxy that read the underlying frame directly, transparently exposing the raw value. Any op that pulls a metadata.common_input field via input.common() (notably reorder_shuffle_by_salt building its salt) then saw the skip value on C++ where Go/Java saw nil, giving a different salt, a different shuffle order, and cascading downstream divergence — the shape nightly diff-fuzz surfaced (seed 3040224764, divergence_000085). Fix: extend InputFieldSpec with an excluded_common set (skip ∪ common_input_skip ∪ common_input_template) and gate OperatorInput::common on it. compute_input_field_spec already built that set locally for bucket exclusion; now it also stores it. common_keys() was implicitly correct — it lists only the three buckets — so no change there. Verified: nightly divergence_000085 now byte-matches Go on C++; pine_cpp_tests 110297/110297; cross-validate sections 3/9 green (97/97, 90/90) with the new dedicated fixture (separate commit) included.
) Dedicated pipeline fixture for the #174 shape: reorder_shuffle_by_salt lists a skip control field (_skip_branch) in metadata.common_input and reads it directly via input.common() to build the salt. All three runtimes must see nil for that slot regardless of whether the operator is actually skipped. Auto-enrolls into cross-validate sections 3/9 via the fixtures/pipelines/*.json glob and pins byte-exact three-runtime output. Verified red on pre-fix pine-cpp (nightly divergence_000085 reproduces), green after the fix; cross-validate 3/9 → 97/97 and 90/90.
Follow-up from #175's fromLua fix (which caught the same luaj coercion trap on the value side): TransformByLua's LuaPool.snapshotKeys collected baseline keys with k.isstring(), which luaj implements with Lua's coercion semantics — LuaInteger.isstring() is unconditionally true, so a numeric global _G[42] gets collected as the phantom string "42". resetToBaseline then calls g.set("42", NIL), which touches the STRING slot _G["42"] while the numeric slot survives. The wangshu godoc already documents the cross-runtime contract: 'baseline reset covers string-keyed globals only; numeric / table / function keys are out of contract.' All three runtimes match this behavior on the observable side (numeric keys leak everywhere). This change is bookkeeping-only: it stops collecting phantom string keys under a coercion query and makes the intent explicit. Normal operator paths never write numeric globals, so no observable behavior changes; the fix closes the last coercion-dispatch site around fromLua. TransformByLuaBaselineTest pins the string-keyed baseline reset contract: a script leaking a string global on borrow 1 must not be visible on borrow 2, and baseline stdlib entries survive reset.
- reference/operator-contract.md: new 'Lua Pool Baseline 重置契约' subsection (wangshu godoc as canonical wording, four-runtime consistent, #177 fixes Java's last coercion predicate site around fromLua). - architecture/dag-engine.md: new 'Operator-visible input 排除集合的跨运行时对齐' subsection (pine-go/pine-java materialize-time exclusion vs pine-cpp proxy read-path gate — same contract, different mechanism; #174). - guides/ci-quality-baseline.md: new 'Artifact triage playbook' subsection (nightly diff-fuzz end-of-pipeline error text often misleads — truncate from tail to find first frame-content divergence; #174 example where op_3 error mask hid op_1 shuffle order divergence). - memory/reflections/skip-field-lazy-input-and-pool-baseline-keys.md: full #174+#177 combined reflection (mistakes, root causes, promotion candidates, fuzz triage playbook). - index.md synced with the four one-liner appends.
Blind review found the earlier TransformByLuaBaselineTest didn't actually pin resetToBaseline: two Registry.buildOperator calls gave two independent LuaPools with independent Globals, so a leak in one was invisible to the other by construction — stubbing resetToBaseline to a no-op left both tests green, and reverting snapshotKeys back to the coercion predicate also stayed green. Real pool reuse needs the same operator instance across two executes. Rewrite the tests around a single op instance so borrow → return → re-borrow actually goes through the reset path: - stringGlobalLeakedByOneBorrowSurvivesInsideSameExecute: ternary-branching script sets a global on first call, must NOT see it on the second. Stubbed resetToBaseline flips the return value. - hijackedTopLevelBaselineGlobalIsRestoredBeforeNextExecute: first call rebinds _G.math to a string sentinel, second must see the restored math table. Scoped to top-level baseline keys — subfield mutation of baseline tables (math.floor = ...) is out of the current baseline-reset contract on the Java runtime (only pine-cpp re-opens safe libs) and out of #177's scope; noted for follow-up. - numericKeyIsIgnoredByBaselineSnapshotRegardlessOfPredicate: direct mechanism test for the #177 predicate switch via reflection on LuaPool.baselineKeys. Init script writes _G[42] before snapshotKeys runs; under the coercion predicate that would land in baselineKeys as phantom "42"; under the fix it stays out. Verified: all three red-before / green-after under two independent mutations (resetToBaseline stubbed to no-op; snapshotKeys reverted to k.isstring()).
Blind review noted this overload no longer has any in-tree caller (transform_redis_get/set both moved to the OperatorInput overload) and calling it would newly bypass the #174 excluded_common gate. Mark it deprecated with a message pointing at the OperatorInput overload and reference issue #174, so any future call site emits a compiler warning before shipping.
…view) Companion to shuffle_salt_reads_skip_field.json: routes the skip field through metadata.common_input_skip (the #74 disjoint bucket) instead of the legacy common_input+skip pattern, pinning the second of three sources merged into pine-cpp's excluded_common set. The third source (common_input_template) routes through templated_param and has separate coverage. Verified byte-identical Go/Java/C++ output; cross-validate section 3 → 98/98, section 9 → 91/91.
…e (review) Blind review found the fixture as first committed didn't uniquely exercise the metadata.common_input_skip source of excluded_common: top-level skip: '_skip_branch' already routed _skip_branch through source #1 (config.skip), and metadata.common_input=[] meant the shuffle op never actually read _skip_branch through input.common, so the gate was never queried. Commenting out the common_input_skip insertion in operator_input.cpp left the fixture output unchanged. Reshape: drop the top-level 'skip' / 'for_branch_control' keys and promote _skip_branch into metadata.common_input alongside metadata.common_input_skip. Now the shuffle op actually reads '_skip_branch' via input.common (which must return nil per the excluded_common gate), and the ONLY source declaring it excluded is metadata.common_input_skip. Verified: three-runtime byte-identical output; commenting the common_input_skip insertion inside compute_input_field_spec now flips the fixture output (source-pin proven).
… (review) The class-level javadoc referenced the earlier draft name 'numericGlobalPollutesTheCorrespondingStringSlotWithCoercionPredicate'; the actual method was renamed to 'numericKeyIsIgnoredByBaselineSnapshotRegardlessOfPredicate' in the prior review pass. Comment-only fix.
Blind review noted the fixture's _comment implied _skip_branch was only in common_input_skip, but the reshape actually places it in BOTH common_input (so shuffle invokes input.common on it) and common_input_skip (the sole exclusion source under test). Rewrite the comment to make that dual placement explicit and note the mutation-test that pins the source. Comment-only.
…urces (review) Blind final review found a pre-existing but same-family issue in snapshot_input (engine.cpp's debug-input snapshot builder): while OperatorInput::common was gated by the full excluded_common set (issue #174 fix), snapshot_input still only filtered on op.skip, missing metadata.common_input_skip and metadata.common_input_template. Result: an op with metadata.common_input_skip=['_skip_branch'] and no top-level 'skip:' leaked '_skip_branch' into [pine-debug] stderr and _return_trace JSON, diverging from pine-go which reads the already- projected input map. The reviewer marked this pre-existing and out-of-#174-scope, but the fix is 2 lines and the same three-source union used by compute_input_field_spec — folding it in here so debug-side and operator-side observation surfaces stay aligned on the same skip contract that dag-engine.md now documents as a cross-runtime invariant. Verified: with the fixture common_input_skip route, pre-fix pine-cpp [pine-debug] output showed input={"common":{"_skip_branch":false},...}; post-fix matches pine-go byte for byte (only items in the input snapshot). pine_cpp_tests still 110297/110297.
Contributor
PR 审查
未发现需要修改的问题；C++ 的 skip/template 排除字段门控、trace 投影和相关调用方迁移，以及 Java Lua global 快照的真实类型标签分派，均与跨运行时契约一致。 风险分级：中 —— 修改运行时算子输入投影和 Lua 池状态重置逻辑，但范围集中，并有针对性 fixture、单元测试与文档覆盖。 完整性声明：据本轮审查，以上为本 PR 已知的全部阻塞级风险；当前未发现阻塞级风险。
|
…nit (CI review) CI diff-fuzz on this PR found a new go-vs-cpp divergence (seed 1784954760 divergence_000076) that the fixture-level fix did not cover. pine-go's SetMetadata (pine.go:159-168) filters skip control fields out of metadata.common_input before handing it to the operator via MetadataAware; the operator sees only business fields. My earlier #174 fix gated OperatorInput::common on the excluded_common set — the skip field's VALUE returns nil — but I never filtered the field NAME out of the list handed to operator init(). Ops that iterate cfg.metadata.common_input to build a salt/hash (reorder_shuffle_by_salt loops over common_inputs_ in execute()) then see an extra entry: Go common_input=[count] salt = "17|" cpp common_input=[count,_skip] salt = "17||" Same downstream cascade as #174: different salt → different shuffle order → cascading downstream item order divergence. Fix: at engine construction, filter cfg.skip out of op_cfg.metadata.common_input before calling op->init(cfg). Matches Go's SetMetadata filter step and applies to every operator uniformly, not just shuffle. DAG dependency inference sees the unfiltered read set via CommonReadFields at plan time, so wiring is unaffected. Verified: the failing CI seed 1784954760 100 rounds is now 100/0; seed 20260725 500 rounds also 100/0; nightly divergence #174/#177 regressions still pass; cross-validate 3/9 → 98/98 + 91/91.
Contributor
PR 审查
未发现需要修改的问题。C++ 的 operator-visible input 排除逻辑覆盖读取、元数据初始化和调试快照等相关消费面；Java 修复使用真实 Lua 类型标签，并通过同一算子实例复用验证池重置契约。 风险分级：中 —— 涉及 C++ 输入投影及 Java Lua 状态池语义，但改动范围明确，并补充了跨运行时 fixture、定向单测和契约文档。 据本轮审查，以上为本 PR 已知的全部阻塞级风险。
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes two related cross-runtime issues caught by nightly diff-fuzz and by follow-up analysis around the pine-java Lua pool bookkeeping.
#174 — pine-cpp
OperatorInput::commonleaked skip / template fieldsNightly diff-fuzz seed 3040224764 caught a Go-vs-C++ divergence:
reorder_shuffle_by_saltlisted a skip control field (_skip_branch) inmetadata.common_inputand built its salt viainput.common(field). pine-go/pine-java materialize thecommonmap at buildInput time and drop skip fields; pre-fix pine-cpp used a lazy proxy that read the underlying frame directly. Different salt → different shuffle order → cascading downstream divergence.Fix: extend
InputFieldSpecwith anexcluded_commonset (union ofconfig.skip,metadata.common_input_skip,metadata.common_input_template) built once bycompute_input_field_spec; gateOperatorInput::commonon it.common_keys()is implicitly correct — it only lists the three buckets, never surfacing the exclusion set.Same three-source fold applied to
snapshot_input(debug-input snapshot builder) so[pine-debug]stderr and_return_traceJSON stay aligned with pine-go.Closes #174.
#177 — pine-java
snapshotKeysused coercion predicateTransformByLua.LuaPool.snapshotKeysfiltered baseline keys withk.isstring(), which luaj implements with Lua coercion semantics —LuaInteger.isstring()is unconditionally true. A script writing_G[42] = ...gets the numeric key collected as the phantom string"42"intobaselineKeys;resetToBaselinethen doesg.set("42", NIL), touching the string slot_G["42"]while the numeric slot survives. The observable behavior (numeric-keyed globals leak across borrows) matches Go/C++/wangshu — all four Lua host libraries treat the baseline reset as string-key-only (wangshu godoc has the canonical wording). But Java was reaching the right behavior via the wrong predicate.Fix: dispatch on
k.type() == LuaValue.TSTRING(same pattern as #175'sfromLuafix). Behavior-neutral on normal operator paths (all host-side globals are string identifiers — function names,set_global(field, ...)calls). This closes the last coercion-dispatch site aroundfromLua.Closes #177.
Regression coverage
Layered per the cross-runtime testing conventions:
TransformByLuaBaselineTest(3 sub-tests) pins the pool-baseline reset contract via same-op reuse + reflection onLuaPool.baselineKeys; verified red-before / green-after under two independent mutations (stubbingresetToBaselineand revertingsnapshotKeystok.isstring()).fixtures/pipelines/shuffle_salt_reads_skip_field.json— pins theconfig.skipsource ofexcluded_common.fixtures/pipelines/shuffle_salt_reads_common_input_skip.json— pins themetadata.common_input_skipsource (dual placement of_skip_branchincommon_inputso the op actually queries the field, and incommon_input_skipas the sole exclusion declaration; commenting out the source Fix ColumnFrame sparse semantics and expand fuzz/concurrency coverage #2 insertion flips this fixture without affecting the sister).Also marked
pine-cpp/operators/_helpers.hpp's legacybuild_key_suffix(Frame&)overload[[deprecated]]with an #174 reference — it has no in-tree caller and would newly bypass theexcluded_commongate.llmdoc
reference/operator-contract.md— new "Lua Pool Baseline 重置契约" subsection (four-runtime string-key-only contract; wangshu godoc as canonical wording; the bug(lua-pool): numeric-keyed globals escape pool baseline reset on all runtimes; pine-java snapshotKeys also uses coercion isstring() (#175 family) #177 predicate fix as the mechanism cleanup).architecture/dag-engine.md— new "Operator-visible input 排除集合的跨运行时对齐" subsection (pine-go/pine-java materialize-time exclusion vs pine-cpp proxy read-path gate — same contract, different mechanism).guides/ci-quality-baseline.md— new "Artifact triage playbook" subsection (nightly diff-fuzz end-of-pipeline error text often misleads; truncate from tail to find first frame-content divergence — the Nightly diff-fuzz: 1 failures, 0 unstable (2026-07-21) #174 shape was hidden behind an op_3 error that was really a downstream cascade of an op_1 shuffle-order divergence).memory/reflections/skip-field-lazy-input-and-pool-baseline-keys.md— full combined reflection.index.md— one-liner appends synced.Local review
Six-run close-local-code-review closure: five blind reviews (run1 REQUEST_CHANGES 1B+2m fixed → run2 REQUEST_CHANGES 1I+2m fixed → run3 APPROVE 1m fixed → run4 APPROVE 0/0/0 → run5 final full-range APPROVE 0B+0I+2m — one folded in-PR, one deferred advisory), plus run6 confirming the in-PR fold with APPROVE 0/0/0, plus terminal audit issuing a PASS receipt over 8 process checks (all digests recomputed, ancestry chain linear, event order correct, finding disposition consistent, final tests re-executed in-tree). See
.code-review/closure-receipt-174-177-run5.md.Testing
pine_cpp_tests238 test cases / 110297 assertions greenmvn test315 tests greengo test ./...green on default backendmake bump VERSION=0.10.16validation: 306 PASS / 0 FAILVersion
Bumped to v0.10.16.