fix: reject operator writes to fields absent from declared outputs (#205) - #209
Open
Liam0205 wants to merge 12 commits into
Open
fix: reject operator writes to fields absent from declared outputs (#205)#209Liam0205 wants to merge 12 commits into
Liam0205 wants to merge 12 commits into
Conversation
The DAG's hazard inference is derived entirely from the declared $metadata field lists (internal/dag.addEdges), so a write to an undeclared field carried no RAW/WAW/WAR edge: nothing ordered it against a concurrent writer of the same name, and a downstream operator declaring that field as input got no dependency on the producer. Read-side projection was already enforced (ComputeInputFieldSpec) and so was the operator-type method restriction (OperatorType.ValidateOutput) — the write-side field name was the one leg of the operator-honesty contract with no enforcement at all (issue #205). Measured affected surface rather than taking the issue's scope (conventions.md "跨运行时缺陷动手前必须实测受影响面"): the mechanism covers FOUR field-carrying write paths, not the two the issue named. SetItemColumnFloat64 and AddItem carry field names too, and AddItem's are the least controlled of all — recall_static derives them from its `items` config param, so an undeclared write is reachable from config alone without any custom operator. recall_static's own doc comment already required set_common keys to be declared; nothing enforced it. Checked at the ValidateOutput call site, not inside ApplyOutput: the frame layer cannot see the declared field lists, but the scheduler already holds cop.Config there. Field names are sorted bytewise so the message does not depend on map iteration order — commonWrites and AddItem payloads are maps, and an order-dependent message cannot be locked byte-exactly across runtimes. The common channel is checked first and returns alone, so a config violating both channels reports only the common fields. Empirically, the full Go suite surfaced 13 violations and every one was a test-only operator writing beyond its declaration; no production or bench operator changed. Two of them were dishonest in a second way: testRecallOp and recallTestOp handed their Init-time maps to AddItem by reference, and ApplyOutput appends that map into the frame and injects `_source` into it, so the operator's cached config accumulated frame state (and downstream SetItem writes) across executions. Production recall operators copy first (operators/recall/static.go); the test operators now do too. `_source` needs no exemption from the check because it is injected during ApplyOutput, after this validation runs — it can only appear in addedItems if the operator reused a map it had already handed over. Note for users: transform_bench_cpu / transform_bench_sleep are registered unconditionally (not build-tag gated), so a config using them must now declare `_bench_result` / `_bench_slept`. Every fixture under fixtures/benchmarks/ already declares what its stubs write, so nothing in this repo needed updating. Refs #205
Two error fixtures for the write-side declared-output contract (issue #205), one per channel, both driven entirely by recall_static so section 05 can run them on all three runtimes without a test-only operator: recall_static takes its item field names from the `items` param and its common field names from `set_common` keys, which is precisely the config-derived path the enforcement protects. wrapping_exact covers go/java/cpp. The common-channel fixture uses two undeclared keys on purpose — it pins the multi-field list shape (bytewise sorted, single-space separated, no quotes or commas, matching the existing type-violation list) and the common-before-item precedence, both of which are the parts most likely to drift between independent reimplementations. Refs #205
Mirror of pine-go types.ValidateDeclaredOutputs (c0b3bf1) for issue #205. OutputContract.validateDeclaredOutputs runs in Engine right after the operator-type method check and before applyOutput, using the declared field lists on the operator config — the same "frame layer cannot see the declaration, but the call site holds it" placement as Go, so the Frame interface is untouched. Contract points that had to be pinned explicitly because Java's defaults diverge from Go's: - Field names sort in UTF-8 byte order via GoFormat.compareUtf8, not String.compareTo (UTF-16 code-unit order disagrees outside the BMP). OutputContractTest pins U+FFFD vs U+10000, which a bare compareTo reverses. - The list renders as Go's %v of a []string (single space, no quotes or commas), identical to the type-violation list Engine already emits. - Common channel first and alone; item channel merges setItem, setItemColumnDouble and addItem field names into one de-duplicated set. `_source` needs no exemption: DataFrame/ColumnFrame.applyOutput copies the added map before injecting it, and the injection runs after this check. Implemented by pa-reviewer (team-member) under the leader-defined contract; mvn test 391/391, checkstyle 0 violations. Red-check: with the Engine call site disabled, the engine-level full-message test goes red. Refs #205
Mirror of pine-go types.ValidateDeclaredOutputs (c0b3bf1) for issue #205. validate_declared_outputs sits in the anonymous namespace right after validate_output_against_type and is called from run_dag immediately after it, before apply_output, using op.metadata — the Frame interface is untouched. std::set<std::string> does both jobs Go needs here: it de-duplicates field names arriving from several write paths (per-element, whole-column, added item) and orders them by std::string::operator<, which compares as unsigned char — the same byte order as Go's sort.Strings — so the message is byte-identical across runtimes. Common channel first and alone, matching Go. `_source` needs no exemption: add_item takes the object by value and RowFrame/ColumnFrame::apply_output inject `_source` into that copy after this check has run. Tests go through Engine::execute end to end with probe operators (the helper is not reachable from the test binary) and compare the full e.what() with ==, covering each write path, common precedence, and cross-path de-dup with the U+FFFD/U+10000 byte-order pair. Implemented by pa-reviewer (team-member) under the leader-defined contract; pine_cpp_tests 260/260, clang-format clean. Red-check: with the run_dag call removed, all six new TEST_CASEs go red. Refs #205
User-visible behaviour change from #205: writes to fields absent from common_output / item_output are now rejected with an ExecutionError on all three runtimes. State the rule next to the Operator interface, name the four write methods it covers, and call out that operators whose field names come from runtime data (recall from config or a resource) are not exempt. Refs #205
…oc-gaps reference/operator-contract.md gains「写侧字段名必须在声明的输出里」— the four write paths, check placement (same site as ValidateOutput, outside the frame), byte-exact message shape, bytewise sort + de-dup, common-before-item precedence, and why `_source` needs no exemption. The SetItemColumnFloat64 consumer checklist and the metadata-consumers list both gain the new check. architecture/dag-engine.md gains「写侧字段名校验」framing the check as the fourth leg of operator honesty (read projection, method class, markers, field names) and why it belongs to the DAG engine rather than API hygiene. must/conventions.md「先实测受影响面」gains #205 as the sixth instance: the issue named two write methods, enumerating the API found four, and the least controlled one (AddItem) sat outside the issue's scope; a team-member's independent fact-check of the issue is the mechanical way to execute "your own issue grants no exemption". memory/doc-gaps.md registers the AddItem map-ownership asymmetry (Go frame appends by reference and mutates the caller's map; Java/C++ copy) surfaced while fixing the test operators. memory/reflections/write-side-declared-outputs-205.md records the process, including the first leader/member two-session collaboration shape. index.md updated for all of the above. Refs #205
…ators map Review finding (recsys-reviewer, minor): dagTestConfig's signature does not show that it edits the caller's operator literals. The helper is idempotent so the side effect is harmless; state it where the helper is defined. Refs #205
…untime Review finding (recsys-reviewer, important): the「写侧字段名」section listed "a reused AddItem map brings _source into the check" under behaviour shared by all three runtimes. Only pine-go's frame appends the caller's map by reference and injects _source into it; Java copies (new LinkedHashMap) and C++ takes add_item by value, so the same reused map passes there forever. Scope the sentence to Go and point at the doc-gaps ownership entry, which already had it right; keep the three-runtime conclusion (custom operators must copy). Refs #205
Contributor
🔍 PR 审查
已核对 Go、Java、C++ 三个运行时的写侧字段声明校验、错误消息一致性、并行输出合并路径及相关测试。未发现阻塞问题、重要建议或小问题。Go 全量测试和 Java Maven 测试通过；少量 Redis/bench fixture 因运行环境条件按预期跳过。 本次审查由 Codex 主链路 (gpt-5.6-sol) 完成。 |
CI benchmark job caught what local go test ./... cannot: pine-go/benchmarks is its own module. makeItems emits item_status and item_category alongside item_id/item_score, and the two recall_static operators declared only the latter pair — exactly the config-derived AddItem shape #205 now rejects. Declaring the extra fields adds no edge between the two recalls (additive writers of the same field stay parallel), so the benchmark still measures what it claims to. Refs #205
…module locally Third time pine-go/benchmarks (independent module) has bitten a PR in a different way (#166 tidy, #160 doc command, #205 behaviour change); the common root is that the main module's ./... never sees it. Add the 1x benchmark run to the step-3c verification list and record the instance in the #205 reflection. Refs #205
Contributor
🔍 PR 审查
未发现需要修改的问题。Go、Java、C++ 三种运行时均在应用输出前覆盖四条带字段名的写路径，并保持错误顺序与文案一致。 验证结果
本次审查由 Codex 主链路 (gpt-5.6-sol) 完成。 |
Review finding (pa-reviewer, minor): "80 余个 benchmark 一分钟内完成" is a count that drifts with every new benchmark and a wall-clock figure, both of which conventions.md「禁止硬编码定量描述」forbids. Keep the command, say "seconds, run-through only". Refs #205
Contributor
🔍 PR 审查
未发现需要修改的问题。Go、Java、C++ 均在应用输出前校验四条带字段名的写路径，校验顺序、排序和错误文案保持一致；新增的单元、集成及跨运行时 fixture 覆盖了声明内外、通道优先级和去重排序场景。 验证通过：Go 本次审查由 Codex 主链路 (gpt-5.6-sol) 完成。 |
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
$metadataoutputs —SetCommonagainstcommon_output;SetItem,SetItemColumnFloat64,AddItemagainstitem_output. Undeclared writes previously bypassed the DAG's hazard graph entirely (no RAW/WAW/WAR edge), sinceaddEdgesderives edges from the declared lists alone.ValidateOutputcall site (after the operator-type method check, before apply), where the operator config is in scope; the Frame interface is untouched. Gotypes.ValidateDeclaredOutputs, JavaOutputContract, C++validate_declared_outputs.fixtures/errors/cases driven by the productionrecall_static(wrapping_exact_engines: go/java/cpp): field names bytewise-sorted and de-duplicated, common channel reported before item, list shape identical to the existing type-violation message.conventions.md): four field-carrying write paths, not the two the issue named; every failing test was a test-only operator, no production or bench operator changed, and allfixtures/benchmarks/already declare what their stubs write. Two test recall operators also handed cached maps toAddItemby reference (frame injects_sourcein place) — fixed to copy like production recalls do.doc/guide_operator{,-en}.md(user-visible rule),llmdocoperator contract / DAG engine / conventions sixth instance / doc-gaps (AddItemmap-ownership asymmetry Go vs Java/C++) / reflection.Validation
go test ./...andgolangci-lintclean; newvalidate_declared_outputs_test.gocovers each write path, sort, de-dup, channel precedence.mvn test391/391 incl.OutputContractTest(13, incl. UTF-8 vs UTF-16 ordering pin); checkstyle clean.pine_cpp_tests260/260 incl. 6 end-to-end probe cases;-Werrorstrict build and clang-format clean.recsys-reviewerfull-range APPROVE (0 blocking / 1 important / 1 minor, both fixed) + incremental APPROVE 0/0/0.Closes #205