Skip to content

ENG-696: structured publish result contract - #286

Open
ea-rus wants to merge 7 commits into
stagingfrom
andrey/eng-696-side-effect-tool-contract-ambiguity
Open

ENG-696: structured publish result contract#286
ea-rus wants to merge 7 commits into
stagingfrom
andrey/eng-696-side-effect-tool-contract-ambiguity

Conversation

@ea-rus

@ea-rus ea-rus commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

What

Companion to the anton PR (ENG-696). Migrates publish_or_preview's action=publish path to return anton's SideEffectResult envelope instead of a prose string:
success / resource_id (report_id) / external_url (view_url) / dempotency_key (report_id) / committed_at / content_hash (md5).

ask/preview and pre-commit validation stay plain strings — they don't commit. Failures use SideEffectResult.failed(...) with a machine reason.

Tests

test_stable_publish_url.py / test_harness_publish_access.py updated to read ToolOutcome.content (same substrings, now inside the envelope's message).

Fixes https://linear.app/mindsdb/issue/ENG-696/side-effect-tool-contract-ambiguity
Should be merged after mindsdb/anton#319

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

No PR environment for this pull request

Add the deploy label and push to create one. It is torn down when the label is removed or the PR closes, so any URL you saw here earlier is gone.

Updated on every push to this PR.

ea-rus and others added 6 commits August 7, 2026 18:11
cowork-server and anton deploy independently and the anton pin can lag the
ENG-696 envelope. Guard the import: when anton.core.tools.side_effect is
absent, return the plain pre-envelope message string (byte-identical to the
old behavior) instead of raising ModuleNotFoundError.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@ea-rus
ea-rus marked this pull request as ready for review August 11, 2026 13:56
@ea-rus
ea-rus requested a review from alecantu7 August 11, 2026 13:56

@alecantu7 alecantu7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adversarial review (deep) — head 3c80ef3e, base origin/staging 936f7083

Reviewed together with anton#319. Deep fan-out: 7 finders across the flagged dimensions, every finding put to 1–3 agents prompted to refute it. 20 findings raised, 8 survived.

The behaviour is sound. The risk I went in expecting — a JSON blob reaching something that string-matched prose — did not materialise: there is no prose-parsing consumer in the renderer, cowork_evals or mdb-ai, and the ImportError shim makes old-anton behaviour byte-identical. What is wrong here is verification, not behaviour.


1. HIGH — the envelope path is unreachable in CI and unpinned by any assertion, so the whole migration reverts green

confirmed · mutation-verified twice by independent reviewers · introduced by this PR

Two halves of one mechanism.

Half A — CI cannot execute the branch. uv.lock:147:

source = { git = "https://github.com/mindsdb/anton.git?branch=main#fdaa2f1d993c062a60daf846b9d1441799977826" }

side_effect.py is absent from anton's origin/main, origin/staging and that locked sha (git cat-file -edoes not exist for all three). tests-unit.yml:30 runs uv sync --group dev against that lock, so tools.py:209 always raises ImportError and takes SideEffectResult = None.

Mutation: inserting raise RuntimeError("MUTANT") after both fallback guards (tools.py:214 and :221) and running the full suite gives 1089 passed, 4 skipped — identical to baseline. Zero tests reach the envelope.

Half B — even when it does run, nothing pins it. With anton#319's side_effect.py force-injected so the branch is live, all 20 publish tests still pass, because every assertion accepts either shape:

tests/test_stable_publish_url.py:256   assert "https://4nton.ai/a/uuid-1" in getattr(out, "content", out)
tests/test_harness_publish_access.py:38 assert "Published" in getattr(out, "content", out)

to_outcome() embeds message inside the JSON, so the substring holds either way. Four mutations against a pristine copy with the envelope live — all survived at 20 passed:

  • revert the migration (if SideEffectResult is Noneif True, both sites)
  • drop resource_id / idempotency_key / content_hash
  • drop committed_at=now_iso()
  • invert both verdicts — every failed publish reports ok=True

That last one is why this is high rather than a test nit. anton/core/session.py:883 does if ok is not None: is_error = not ok, and its own docstring names the consequence: "a genuine failure … RESETS it, which is how the ENG-836 driver ping-pong … kept the breaker asleep at ~4.95M tokens." An inverted ok on publish silently disarms the ENG-1276 circuit breaker — the mechanism ENG-696 exists to arm.

The new test_tool_publish_falls_back_to_string_on_old_anton is a good test (deleting the try/except does fail it) but it covers the branch CI already takes by default, not the deliverable.

And it activates without a gate. The shipped wheel carries no git pin — Requires-Dist: anton-agent<3,>=2.26.8.9.1 — and publish-staging.yml:101-108 rewrites the dep to the latest PyPI rc at publish time. So the untested branch goes live on anton's next release: no lock bump, no PR, no review.

Smallest fix — ~20 lines, needs no anton merge. Invert the patch.dict the PR already uses: patch the module to a stub rather than to None.

def test_tool_publish_returns_envelope_on_new_anton(tmp_path):
    stub = types.ModuleType("anton.core.tools.side_effect")
    stub.SideEffectResult = SideEffectResult      # or a minimal local dataclass
    stub.now_iso = lambda: "2026-08-11T00:00:00Z"
    with patch.dict(sys.modules, {"anton.core.tools.side_effect": stub}), \
         patch.object(tools_mod, "_publish_artifact",
             lambda p, access=None: {"url": "https://4nton.ai/a/uuid-1",
                                     "result": {"report_id": "rep-1", "md5": "deadbeef"}}):
        out = _run(tools_mod._cowork_publish_or_preview(_FakeSession(tmp_path), {...}))
    payload = json.loads(out.content)
    assert out.ok is True
    assert payload["resource_id"] == "rep-1" and payload["idempotency_key"] == "rep-1"
    assert payload["external_url"] == "https://4nton.ai/a/uuid-1"
    assert payload["content_hash"] == "md5:deadbeef" and payload["committed_at"]

Plus a failure twin asserting out.ok is False and out.reason == "missing_api_key". Note _publish_artifact has to be stubbed to return the nested {"result": {"report_id", "md5"}} shape — no current test ever makes those non-empty, so the field mapping is unexercised even in principle.

Fails before / passes after: the pair fails against all four mutations above (the revert mutant returns a str, so out.content raises AttributeError; the inverted mutant fails out.ok is True) and passes on HEAD with the stub — independent of which anton is installed.


2. MEDIUM — the envelope activates on four channels at four different times, and hosted never activates on merge

confirmed · pre-existing topology, first consequential here · no code fix — PR-body + QA item

  • Hosted/Docker: frozen. Dockerfile:26,31 uv sync --frozen → the locked fdaa2f1, which has no side_effect.py. Nothing forces a bump: rewriting the pin to an older commit still passes uv lock --check in 12ms, because pyproject.toml:79 only requires branch = "main".
  • Desktop staging: first. cowork/src/main/server-source.ts:196-215 lets cowork-server's own pin decide, and build-macos-pkg.yml:252-256 passes ANTON_REF, so a staging build resolves anton@staging and picks up #319 the moment it merges — ahead of main.
  • Desktop main / PyPI: on anton's next release.

No functional breakage — the fallback keeps publishing byte-identical. But "merge #319, then verify the envelope" will be verified against a build still emitting prose and read as a failed fix. That is exactly the team's QA Step 0 shape.

Suggested: QA Step 0 should say "confirm which anton the build carries before judging output shape" — the signal already exists and is free: cowork/api/v1/endpoints/health.py:35 returns anton_version, and build_info.py:147-151 stamps it on every Langfuse trace. Add to expected side effects that are NOT regressions: a prose-string publish means an old pin, not a failed fix.


Checked and produced nothing

  • Prose consumers. Inventoried exhaustively across cowork, cowork_evals and mdb-ai and structurally immune: anton emits tool content only for scratchpad (session.py:3552, :3699, both name-guarded); the generic branch yields StreamTaskProgress with no content. Artifact cards come from a directory diff (harness.py:361), the "Shared" pill from the REST sidecar, persisted tool rows are hidden (conversations.py:575). Grep for every removed prose string (PUBLISH FAILED, Published successfully, no view URL, No Minds API key, Artifact store unavailable) → zero hits. An executed probe drove the real _cowork_publish_or_preview through dispatch_tool into format_responses_stream: envelope in SSE: False. Worth one line in the PR body so a reviewer doesn't re-litigate it.
  • Desktop timeline turning failed publishes red. Refuted: stream_formatter.py:321 gates thought.tool_call.end on event.id in progress_tool_ids, populated only by tool_progress, which no production handler emits. ok never reaches the renderer for these tools.
  • publish's first-publish idempotency window. Real pre-existing gap (server-mints report_id, persisted only after a successful round-trip; publish.py:350-353 swallows the write failure) but wholly outside this diff, and the envelope emits explicit null there rather than a false claim. Separate ticket.
  • -> str removed rather than widened on _cowork_publish_or_preview. Style only — registry.py:104-114 documents str | ToolOutcome as the handler contract and normalises both; no type-checker runs in CI.

Recommendation: the top finding is worth changes requested — one stub test closes it and costs ~20 lines. Leaving this as a COMMENT so the formal state stays a human call.

Merge order: this PR's body already says it should land after anton#319 — worth noting that means after #319 reaches anton main, not staging, since that is what the lock resolves.

@alecantu7 alecantu7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes on one item — the behaviour is right, the verification isn't there.

The envelope path is never executed, and nothing pins it. Detail and evidence in my review above; the short version:

  • uv.lock:147 pins anton at fdaa2f1, which has no side_effect.py — so CI always takes the ImportError fallback. Inserting raise RuntimeError("MUTANT") after both guards (tools.py:214, :221) still gives 1089 passed, 4 skipped — identical to baseline.
  • Force-injecting the module so the branch is live, all 20 publish tests still pass under four separate sabotage mutations: reverting the migration; dropping resource_id/idempotency_key/content_hash; dropping committed_at; and inverting both ok verdicts. Every assertion is getattr(out, "content", out) plus a substring, and to_outcome() embeds message inside the JSON, so the substring holds either way.

The inverted-verdict case is why this is blocking rather than a nit: anton/core/session.py:883 does is_error = not ok, so a publish failure reporting ok=True silently disarms the ENG-1276 circuit breaker — the mechanism ENG-696 exists to arm. And it activates without a gate: the shipped wheel carries no git pin, and publish-staging.yml:101-108 rewrites the dep to the latest PyPI rc, so the untested branch goes live on anton's next release.

~20 lines closes it, and it needs no anton merge — invert the patch.dict this PR already uses: patch the module to a stub rather than to None, assert out.ok and the five envelope fields on the success path, plus a failure twin asserting out.ok is False and out.reason == "missing_api_key". Full snippet in the review above. Note _publish_artifact has to be stubbed to return the nested {"result": {"report_id", "md5"}} shape — no current test makes those non-empty, so the field mapping is unexercised even in principle.

Nothing else on the PR is blocking. The prose→JSON switch is safe (no consumer anywhere string-matched the old format — verified by executing the real dispatch path and grepping every removed prose string across cowork, cowork_evals and mdb-ai), and the ImportError shim makes old-anton behaviour byte-identical.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants