Conversation
hetaoBackend
left a comment
There was a problem hiding this comment.
Request changes for exact current head 35223d3.
Blocking correctness/contract issues:
reuseAcrossRunsis implemented inengine.mjs:48,77, but is absent from the public MCP schemas insrc/tools.mjs:13-14(and the generateddist/main.mjs). Those schemas useadditionalProperties:false, so a normal MCP client cannot discover or reliably send the documented feature. Add the field and description toworkflow_startandworkflow_update, rebuild dist, and add a packaged MCP contract test that enables it through the public tool surface.- Cross-run reuse does not track upstream lineage.
engine.mjs:199-208andstore.mjs:52match only the downstream node request hash and a run-level context hash;dependsOnis only the upstream ID. I reproduced this on the exact head: run 1 executeda(prompt=old) -> b(prompt=same), run 2 executeda(prompt=new)and then reusedbfrom run 1, producinga=out:newwithb=out:sameandreusedFrom.crossRun=true. A changed/rerun upstream must invalidate downstream reuse, as the README promises. Persist/compare effective dependency lineage or upstream output/spec hashes, and add the R1/R2 regression test. - For MCode nodes without an explicit
spec.model, the effective default model/provider/config comes from the CLI environment, but the cache identity contains onlyexecutor: 'mcode'and the node spec. A changed MCode default model/config/CLI can therefore reuse an old result. Require an explicit model for cross-run MCode reuse or include a resolved model/config/CLI identity in the cache key and test that a default-model change misses. store.findCrossRunReuse()appliesLIMIT 20before filtering by context hash (store.mjs:52), so 20 newer same-node results from other contexts can hide an older valid candidate forever. Filter in SQL using a persisted/indexed context key or scan/paginate until a valid candidate is found.- Cross-run adoption of a repair-reused step preserves the old
reusedFromobject (engine.mjs:205-206) while emitting acrossRun:trueevent (:207), so stored provenance can disagree with the event. Preserve immediate source and original producer as separate provenance fields and add a repair-to-cross-run test.
Current GitHub checks are green, but they do not cover the public MCP schema path or the reproduced stale-lineage case. Rebase/rebuild against the current main before requesting another review.
|
Review of current head
Validation: the new cross-reuse suite passes 11/11 locally. The failing lineage probe exercises orchestration with deterministic outputs and real temporary-file I/O; no paid model execution or model-quality claim is involved. |
35223d3 to
8df53c9
Compare
|
All four items addressed at exact head
Also inherited from your first review and included here: mcode nodes without an explicit Validation at this head: 90/90 plugin checks (72 baseline + 15 cross-reuse incl. both repro shapes + MCP contract + package smoke), byte-reproducible rebuild, and the full 493-test repository gate on a local Ubuntu 24.04 box — 482 pass / 0 fail / 11 platform skips. Re-requesting review. |
|
Correction: exact head is now |
|
Re-reviewed at One remaining issue before merging: cross-run adoption retains the source run's Independent reproduction with the real Engine and
Please overwrite the adopted step's Latest-head CI is green. This was reproduced with a deterministic executor and the actual graph-model function; it was not a browser screenshot or real-model test. |
|
Fixed at exact head |
|
Re-reviewed exact head 1. [P1] An upstream rerun with unchanged returned text can still reuse a stale downstream result
Independent reproduction with the actual Engine/Store, a deterministic executor, and real temporary-file I/O:
The changed-output regression does not cover this unchanged-output case. Please invalidate downstream candidates when a relevant upstream agent was freshly executed, or bind enough execution/artifact identity to establish equivalence safely. Add this same-output, changed-tracked-file regression with B's spec unchanged. 2. [P2] Multiple repairs followed by cross-run reuse lose the original producerRepair adoption at Independently reproduced:
R4 correctly records Validation at this exact head: plugin checks 91/91, packaged MCP smoke 1/1, byte-identical rebuild, and clean-tree repository validator pass. These additional reproductions use real orchestration/storage and deterministic execution; they do not involve real model calls. Separately, current exact-head GitHub checks are 4 passed / 1 failed / 1 skipped. Ubuntu CI run 35318580789 fails |
|
On the red |
- run-level reuseAcrossRuns flag (default false, part of requestHash) - agent() consults prior succeeded nodes from other runs in the same workspace store when context (workspace/input/executor/fingerprints) and spec hash match; newest first, schema revalidated per candidate - reused results carry reusedFrom provenance (crossRun marker, original producer preserved) and consume no call budget - store.findCrossRunReuse joins steps x runs with json_extract filters - README documents the semantics and the honest boundary: reuse proves context identity and storage fidelity, not first-run correctness
…ovenance - reuseAcrossRuns added to workflow_start/workflow_update MCP schemas (additionalProperties:false previously made the flag undiscoverable) - lineageHash per step (requestHash + dependency lineage closure); cross-run lookup requires lineage equality — a changed/rerun upstream now invalidates downstream reuse (maintainer's reproduced R1/R2 case, regression-tested) - mcode nodes without an explicit spec.model are never cross-run candidates (resolved CLI default model/config is not part of any cache key) - contextHash persisted on steps and filtered in SQL; LIMIT 20 can no longer hide older valid candidates behind newer other-context rows - adoption always writes immediate-source reusedFrom (crossRun:true) plus originalProducer (first producer), matching the emitted event - checkpoints carry lineageHash so value changes break downstream lineage
…dd MCP contract test - store.mjs rebuilt from main: findCrossRunReuse only, no integrity-ledger code (the ledger belongs to MiniMax-AI#48; the previous round accidentally carried it) - lineageHash now includes each succeeded dependency's output hash, so an upstream that re-executes with different output (mcode node without an explicit model, tracked-file change during execution) invalidates downstream adoption — regression covers the maintainer's divergence shape - checks/cross-reuse-mcp.check.mjs: packaged MCP advertises reuseAcrossRuns on workflow_start/workflow_update and accepts/rejects it through the public tool surface (additionalProperties:false contract)
These are contributor-local gate runners (local mirror + remote Ubuntu box), not part of the plugin or the repository contract. They slipped in via a broad 'git add -A' during the r2 fix round.
The adopted step spread the source run's planId, so a reused node kept the old topology's plan anchor while the current run's topology carried its own planId for the same agent — the dashboard then rendered two nodes (one planned-not-run, one live). Adoption now restamps the current dispatch's planId exactly like the repair path; regression asserts the adopted step maps onto the current topology's single node (maintainer's workflowGraph reproduction).
accae5c to
db2dafb
Compare
|
Rebased onto main after #50 (and #45's merge); exact head is now |
…r chains - cross-run adoption now requires every agent dependency to be itself adopted/reused: a freshly executed upstream has unproven execution identity — spec and output hashes cannot see changed filesystem effects (same return value, different written content). Checkpoints recompute deterministically and stay eligible via their value hash in lineage. Regression mirrors the maintainer's same-output/changed-tracked-file repro end-to-end with real file I/O (a re-executes returning 'done', b re-executes and reads the new content). - repair adoption now carries originalProducer (first producer relayed through R2/R3 repairs, not reset to the immediate repair source), so a later cross-run adoption resolves originalProducer.runId to the run that actually executed the node (multi-repair chain regression)
|
Both findings fixed at exact head
Suite 102/102, packaged smoke 1/1, rebuild byte-reproducible, rebased on the post-#50 main (validate deterministic). |
What changes
Implements opt-in content-addressed cross-run reuse for the dynamic-workflow plugin: a run can now adopt succeeded nodes from any prior run in the same workspace store when both the execution context and the call spec hash identically — not only within a repair lineage.
Design provenance: the derived-stable-identifier approach ("the key is the identity") follows our deterministic-addressing tool (sih-engine family,
sih-tools/locator).Design
reuseAcrossRuns: trueonstart/update(default false). The key is absent from the definition unless explicitly true — an always-present default key would changerequestHashand break the legacy idempotent-replay path (caught by the "legacy limits … old idempotent requests still work" test; the fix follows the same absent-means-default pattern asDEFAULT_LIMITS).hash({workspace, input, executor, fingerprints})(same four factors as repair's contextHash, cached per run), spec =hash(spec)(prompt/schema/input included — a different schema is a different contract). Lookup joinssteps × runsviajson_extract, newest first, this run excluded, onlysucceededagent nodes.reusedFrom— the original producer's lineage when present, otherwise{runId, stepId, endedAt, crossRun: true}— and emitstep.reused. Reuse consumes no call budget (attempt: 0, before the budget gate).Test evidence
checks/cross-reuse.check.mjs: 11 tests — default-off, opt-in hit (zero new calls, provenance, event), input/executor/prompt/schema/fingerprint changes all miss, failed nodes never reused, newest-wins, pure reuse on a one-call budget, requestId idempotency across a flipped flag.npm run buildbyte-reproducible; repository validator green on a clean checkout.main(00878c7).Follow-up seam
Once #48 (integrity ledger) merges, a small follow-up can route accepted cross-run reuses through the ledgered surface so "what reuse trusts" is covered by the same tamper-evidence chain.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.