From 1aa81c711e1bd59893fdcdf2c49860add023fe92 Mon Sep 17 00:00:00 2001 From: Abidoyesimze Date: Thu, 15 Oct 2026 10:07:56 +0100 Subject: [PATCH 1/3] fix(dincli): detect partial create-testdataset submit failures (issue #252) Previously, assignAuditTestDataset tx results were discarded in the --submit loop and setTestDataAssignedFlag was sent unconditionally afterwards, so a failed batch silently left auditors unable to decrypt their test data while the command still exited 0. Now each batch's tx receipt is collected: on any failure, the failed batch IDs are printed and the flag tx is skipped, exiting 1. A new repeatable --batch option retries only the listed batches. The flag send is preceded by a static call to distinguish an already-set revert (informational, skipped) from any other failure (exit 1), since the flag has no getter to check first. Co-Authored-By: Claude Sonnet 5 --- .../public/workflows/model-workflow.md | 8 + dincli/cli/modelownerd/auditor_batches.py | 86 +++++- tests/test_create_testdataset_submit.py | 285 ++++++++++++++++++ 3 files changed, 371 insertions(+), 8 deletions(-) create mode 100644 tests/test_create_testdataset_submit.py diff --git a/Documentation/public/workflows/model-workflow.md b/Documentation/public/workflows/model-workflow.md index 3916949c..765d2b14 100644 --- a/Documentation/public/workflows/model-workflow.md +++ b/Documentation/public/workflows/model-workflow.md @@ -360,6 +360,14 @@ dincli model-owner lms-evaluation show --auditors --models > where `CACHE_DIR` is the path to the cache directory of the dincli. > and can be found by running the command `dincli system get-cache-dir`. +> [NOTE] +> `create-testdataset --submit` is safe to re-run. If some auditor batches failed to +> be assigned, it prints their batch IDs, skips the test-dataset-assigned flag, and +> exits non-zero. Retry just those batches with the repeatable `--batch ` option, +> e.g. `dincli model-owner auditor-batches create-testdataset --submit --batch 2 --batch 5`. +> The flag itself is one-shot on-chain; re-running after it's already set is detected +> and skipped rather than treated as an error. + **Auditors** evaluate their assigned batches (repeat for each auditor): ```bash diff --git a/dincli/cli/modelownerd/auditor_batches.py b/dincli/cli/modelownerd/auditor_batches.py index 7babe820..65e03130 100644 --- a/dincli/cli/modelownerd/auditor_batches.py +++ b/dincli/cli/modelownerd/auditor_batches.py @@ -1,6 +1,7 @@ import secrets from pathlib import Path import time +from typing import List import typer from rich.table import Table from web3 import Web3 @@ -15,6 +16,24 @@ auditor_batches_app = typer.Typer(help="Auditor Batches commands") +_TA_FLAG_ALREADY_SET_SELECTOR = Web3.keccak(text="TA_FlagAlreadySet()").hex()[:8].lower() + + +def _is_flag_already_set_revert(exc: Exception) -> bool: + """Best-effort detection of DINTaskAuditor's TA_FlagAlreadySet() custom error. + + setTestDataAssignedFlag has no getter for Is_testdataCIDs_Assigned, so + inspecting the revert data is the only way to tell "already set" (safe to + skip) apart from a real failure (should exit non-zero). + """ + data = getattr(exc, "data", None) + if isinstance(data, dict): + data = data.get("data") + if not isinstance(data, str): + return False + data_hex = data[2:] if data.startswith("0x") else data + return data_hex.lower().startswith(_TA_FLAG_ALREADY_SET_SELECTOR) + @auditor_batches_app.command() def create( ctx: typer.Context, @@ -124,7 +143,25 @@ def create_testdataset( gi: int = typer.Option(None, "--gi", help="Global iteration number"), submit: bool = typer.Option(False, "--submit", help="Submit test dataset to TaskCoordinator"), test_data_path: str = typer.Option(None, "--test-data-path", help="Path to test dataset"), + batch: List[int] = typer.Option( + [], + "--batch", + help=( + "Retry assignAuditTestDataset for only this batch ID (repeatable, e.g. " + "--batch 2 --batch 5). Combine with --submit to retry batches that failed " + "on a previous run; the test-dataset-assigned flag is then (re-)attempted " + "for all batches as usual. If omitted, --submit sends every batch." + ), + ), ): + """Create per-batch encrypted audit test datasets and optionally submit them on-chain. + + Safe to re-run with --submit: if any batch's assignAuditTestDataset tx fails, + the failed batch IDs are printed and the command exits 1 without touching the + test-dataset-assigned flag. Retry just those batches with `--batch ` + (repeatable). The flag itself is one-shot on-chain; if it's already set, + re-running --submit (with or without --batch) detects that and skips it. + """ effective_network, w3, account, console = ctx.obj.get_en_w3_account_console(model_id) taskCoordinator_contract, taskauditor_contract = ctx.obj.get_deployed_din_task_coordinator_contract(True, model_id), ctx.obj.get_deployed_din_task_auditor_contract(True, model_id) @@ -194,8 +231,11 @@ def create_testdataset( stake_contract = ctx.obj.get_deployed_din_stake_contract(verbose=False) + batch_ids_to_process = list(batch) if batch else list(range(audtor_batch_count)) + + failed_batch_ids = [] try: - for batch_id in range(audtor_batch_count): + for batch_id in batch_ids_to_process: batch_info = audit_testDataCIDs[batch_id] raw_cid = batch_info["raw_cid"] K = batch_info["K"] @@ -231,7 +271,7 @@ def create_testdataset( )) time.sleep(5) - build_and_send_tx( + tx_receipt = build_and_send_tx( ctx, taskauditor_contract.functions.assignAuditTestDataset( curr_GI, batch_id, encrypted_cid, encrypted_keys, commitment @@ -241,9 +281,40 @@ def create_testdataset( f"Failed to assign test dataset for auditor batch : {batch_id}", exit_on_failure=False, ) + if tx_receipt is None: + failed_batch_ids.append(batch_id) + except Exception as e: + console.print(f"[red]Error:[/red] Failed to assign test dataset for auditor batches: {e}") + raise typer.Exit(1) + + if failed_batch_ids: + retry_flags = " ".join(f"--batch {b}" for b in failed_batch_ids) + console.print( + f"[bold red]✗ Failed to assign test dataset for auditor batch(es): " + f"{', '.join(str(b) for b in failed_batch_ids)}[/bold red]" + ) + console.print( + f"[yellow]Skipping the test-dataset-assigned flag. Retry just the failed " + f"batch(es) with: dincli model-owner task auditor-batches create-testdataset " + f"{model_id} --submit {retry_flags}[/yellow]" + ) + raise typer.Exit(1) - time.sleep(5) - build_and_send_tx( + time.sleep(5) + flag_already_set = False + try: + taskCoordinator_contract.functions.setTestDataAssignedFlag(curr_GI, True).call({"from": account.address}) + except Exception as e: + if _is_flag_already_set_revert(e): + flag_already_set = True + else: + console.print(f"[bold red]✗ Failed to set test dataset assigned flag: {e}[/bold red]") + raise typer.Exit(1) + + if flag_already_set: + console.print("[yellow]Test dataset assigned flag was already set for this GI — skipping.[/yellow]") + else: + flag_receipt = build_and_send_tx( ctx, taskCoordinator_contract.functions.setTestDataAssignedFlag(curr_GI, True), "Setting test dataset assigned flag", @@ -251,7 +322,6 @@ def create_testdataset( "Failed to set test dataset assigned flag", exit_on_failure=False, ) - except Exception as e: - console.print(f"[red]Error:[/red] Failed to assign test dataset for auditor batches: {e}") - raise typer.Exit(1) - \ No newline at end of file + if flag_receipt is None: + console.print("[bold red]✗ Failed to set test dataset assigned flag.[/bold red]") + raise typer.Exit(1) diff --git a/tests/test_create_testdataset_submit.py b/tests/test_create_testdataset_submit.py new file mode 100644 index 00000000..76c2f0c1 --- /dev/null +++ b/tests/test_create_testdataset_submit.py @@ -0,0 +1,285 @@ +import secrets +from types import SimpleNamespace + +import pytest +import typer +from nacl.public import PrivateKey +from web3 import Web3 + +from dincli.cli.modelownerd import auditor_batches as atb + + +_TA_FLAG_ALREADY_SET_SELECTOR = Web3.keccak(text="TA_FlagAlreadySet()").hex()[:8] + +_AUDITOR_PRIVKEY = PrivateKey.generate() +_AUDITOR_PUBKEY_BYTES = bytes(_AUDITOR_PRIVKEY.public_key) +_AUDITOR_ADDRESS = "0xAuditorOne" + + +def _fake_get_bytes32_from_cid(cid_str: str) -> str: + import hashlib + + return hashlib.sha256(cid_str.encode()).hexdigest() + + +class FakeRevert(Exception): + def __init__(self, data): + super().__init__("execution reverted") + self.data = data + + +def flag_already_set_revert(): + return FakeRevert(data="0x" + _TA_FLAG_ALREADY_SET_SELECTOR) + + +def other_revert(): + return FakeRevert(data="0xdeadbeef") + + +class DummyConsole: + def __init__(self): + self.messages = [] + + def print(self, *args, **kwargs): + self.messages.append(" ".join(str(a) for a in args)) + + +class DummyCall: + def __init__(self, value): + self.value = value + + def call(self, *args, **kwargs): + return self.value + + +class DummyTaskAuditorFunctions: + def __init__(self, contract): + self.contract = contract + + def AuditorsBatchCount(self, gi): + return DummyCall(self.contract.batch_count) + + def getAuditorsBatch(self, gi, batch_id): + return DummyCall((batch_id, [_AUDITOR_ADDRESS], [], b"")) + + def assignAuditTestDataset(self, gi, batch_id, encrypted_cid, encrypted_keys, commitment): + return ("assignAuditTestDataset", batch_id) + + +class DummyTaskAuditorContract: + def __init__(self, batch_count): + self.batch_count = batch_count + self.functions = DummyTaskAuditorFunctions(self) + + +class _SetFlagCall: + def __init__(self, contract): + self.contract = contract + + def call(self, tx_params=None): + if self.contract.flag_call_raises is not None: + raise self.contract.flag_call_raises + return None + + +class DummyTaskCoordinatorFunctions: + def __init__(self, contract): + self.contract = contract + + def setTestDataAssignedFlag(self, gi, value): + return _SetFlagCall(self.contract) + + +class DummyTaskCoordinatorContract: + def __init__(self, gi=0, gi_state=0, flag_call_raises=None): + self.gi = gi + self.gi_state = gi_state + self.flag_call_raises = flag_call_raises + self.functions = DummyTaskCoordinatorFunctions(self) + + +class DummyStakeFunctions: + def getEncryptionKey(self, address): + return DummyCall(_AUDITOR_PUBKEY_BYTES) + + +class DummyStakeContract: + def __init__(self): + self.functions = DummyStakeFunctions() + + +class DummyAccount: + address = "0xOwner" + + def sign_message(self, message): + return SimpleNamespace(signature=secrets.token_bytes(65)) + + +class DummyContextObj: + def __init__(self, tmp_path, task_coordinator_contract, task_auditor_contract): + self.console = DummyConsole() + self.account = DummyAccount() + self.w3 = object() + self.tmp_path = tmp_path + self.task_coordinator_contract = task_coordinator_contract + self.task_auditor_contract = task_auditor_contract + self.stake_contract = DummyStakeContract() + + def get_en_w3_account_console(self, *args, **kwargs): + return "local", self.w3, self.account, self.console + + def get_deployed_din_task_coordinator_contract(self, *args, **kwargs): + return self.task_coordinator_contract + + def get_deployed_din_task_auditor_contract(self, *args, **kwargs): + return self.task_auditor_contract + + def get_current_gi_and_state(self, contract, *args, **kwargs): + return contract.gi, contract.gi_state + + def validate_gi_ET_curr_GI(self, gi, curr_gi): + return curr_gi + + def validate_GIstate_ET_given_GIstate(self, *args, **kwargs): + return None + + def get_model_base_dir(self, model_id): + return self.tmp_path + + def ensure_file_exists(self, *args, **kwargs): + return None + + def load_custom_fn(self, *args, **kwargs): + def fake_fn(batch_count, gi, model_base_path, test_data_path): + return [ + { + "raw_cid": f"cid-{i}", + "K": secrets.token_bytes(32), + "plaintext_keccak": b"\x00" * 32, + } + for i in range(batch_count) + ] + + return fake_fn + + def get_deployed_din_stake_contract(self, *args, **kwargs): + return self.stake_contract + + +def make_ctx(tmp_path, batch_count=3, flag_call_raises=None): + task_coordinator_contract = DummyTaskCoordinatorContract(flag_call_raises=flag_call_raises) + task_auditor_contract = DummyTaskAuditorContract(batch_count) + ctx_obj = DummyContextObj(tmp_path, task_coordinator_contract, task_auditor_contract) + return SimpleNamespace(obj=ctx_obj), task_coordinator_contract, task_auditor_contract + + +@pytest.fixture(autouse=True) +def patch_module(monkeypatch, tmp_path): + monkeypatch.setattr(atb, "get_bytes32_from_cid", _fake_get_bytes32_from_cid) + monkeypatch.setattr(atb, "get_manifest_key", lambda *a, **kw: { + "type": "custom", "path": "create_audit_testDataCIDs.py", "ipfs": "cid-service", + }) + monkeypatch.setattr(atb, "CONFIG_DIR", str(tmp_path / "config")) + (tmp_path / "config").mkdir(parents=True, exist_ok=True) + monkeypatch.setattr( + "dincli.cli.utils.get_manifest_path", lambda *a, **kw: tmp_path / "no_manifest.json" + ) + monkeypatch.setattr(atb.time, "sleep", lambda seconds: None) + + +def _invoke(tmp_path, ctx, model_id=1, gi=None, submit=True, test_data_path=None, batch=None): + test_data_file = tmp_path / "test_dataset.pt" + test_data_file.write_bytes(b"dummy") + atb.create_testdataset( + ctx, + model_id=model_id, + gi=gi, + submit=submit, + test_data_path=test_data_path or str(test_data_file), + batch=batch or [], + ) + + +def test_all_batches_succeed_sends_flag(monkeypatch, tmp_path): + ctx, coordinator, auditor = make_ctx(tmp_path, batch_count=3) + + sent = [] + + def fake_build_and_send_tx(ctx, contract_function, action_msg, success_msg, error_msg, *a, **kw): + sent.append(contract_function) + return SimpleNamespace(transactionHash=b"\xab" * 32) + + monkeypatch.setattr(atb, "build_and_send_tx", fake_build_and_send_tx) + + _invoke(tmp_path, ctx) + + assign_calls = [c for c in sent if isinstance(c, tuple) and c[0] == "assignAuditTestDataset"] + assert sorted(c[1] for c in assign_calls) == [0, 1, 2] + # last sent call is the flag tx (a _SetFlagCall instance, not our assign tuple) + assert any(not isinstance(c, tuple) for c in sent) + + +def test_one_batch_fails_skips_flag_and_exits(monkeypatch, tmp_path): + ctx, coordinator, auditor = make_ctx(tmp_path, batch_count=3) + + sent = [] + + def fake_build_and_send_tx(ctx, contract_function, action_msg, success_msg, error_msg, *a, **kw): + sent.append(contract_function) + if isinstance(contract_function, tuple) and contract_function[1] == 1: + return None + return SimpleNamespace(transactionHash=b"\xab" * 32) + + monkeypatch.setattr(atb, "build_and_send_tx", fake_build_and_send_tx) + + with pytest.raises(typer.Exit): + _invoke(tmp_path, ctx) + + assign_calls = [c for c in sent if isinstance(c, tuple) and c[0] == "assignAuditTestDataset"] + assert sorted(c[1] for c in assign_calls) == [0, 1, 2] + # flag must never be sent (would be a non-tuple contract_function) + assert all(isinstance(c, tuple) for c in sent) + assert any("1" in msg for msg in ctx.obj.console.messages) + + +def test_batch_retry_only_sends_listed_batches_then_flag(monkeypatch, tmp_path): + ctx, coordinator, auditor = make_ctx(tmp_path, batch_count=3) + + sent = [] + + def fake_build_and_send_tx(ctx, contract_function, action_msg, success_msg, error_msg, *a, **kw): + sent.append(contract_function) + return SimpleNamespace(transactionHash=b"\xab" * 32) + + monkeypatch.setattr(atb, "build_and_send_tx", fake_build_and_send_tx) + + _invoke(tmp_path, ctx, batch=[1]) + + assign_calls = [c for c in sent if isinstance(c, tuple) and c[0] == "assignAuditTestDataset"] + assert [c[1] for c in assign_calls] == [1] + assert any(not isinstance(c, tuple) for c in sent) + + +def test_flag_already_set_is_informational_not_error(monkeypatch, tmp_path): + ctx, coordinator, auditor = make_ctx(tmp_path, batch_count=2, flag_call_raises=flag_already_set_revert()) + + def fake_build_and_send_tx(ctx, contract_function, action_msg, success_msg, error_msg, *a, **kw): + return SimpleNamespace(transactionHash=b"\xab" * 32) + + monkeypatch.setattr(atb, "build_and_send_tx", fake_build_and_send_tx) + + _invoke(tmp_path, ctx) + + assert any("already set" in msg.lower() for msg in ctx.obj.console.messages) + + +def test_flag_other_revert_exits_nonzero(monkeypatch, tmp_path): + ctx, coordinator, auditor = make_ctx(tmp_path, batch_count=2, flag_call_raises=other_revert()) + + def fake_build_and_send_tx(ctx, contract_function, action_msg, success_msg, error_msg, *a, **kw): + return SimpleNamespace(transactionHash=b"\xab" * 32) + + monkeypatch.setattr(atb, "build_and_send_tx", fake_build_and_send_tx) + + with pytest.raises(typer.Exit): + _invoke(tmp_path, ctx) From 5741597aae394b455889bed053f288a9117d37d0 Mon Sep 17 00:00:00 2001 From: Abidoyesimze Date: Sat, 24 Oct 2026 20:32:17 +0100 Subject: [PATCH 2/3] fix(dincli): correct retry command typo, add CliRunner coverage (PR 258 review) Fixes two items from review: 1. The printed retry hint on partial create-testdataset submit failure said "dincli model-owner task auditor-batches create-testdataset", but auditor-batches is registered directly under model-owner, not under task. Drop the extra "task" so the suggested command can be copied and run as-is. 2. The existing tests called create_testdataset directly, bypassing the registered CLI's argument parsing -- exactly the layer that hid the typo above. Add CliRunner-based tests that invoke the real `dincli model-owner auditor-batches create-testdataset` command for all-succeed, partial-failure (asserting exit code 1 and the exact retry command text), repeatable --batch retry, and both already-set/other-revert flag outcomes. Co-Authored-By: Claude Sonnet 5 --- dincli/cli/modelownerd/auditor_batches.py | 2 +- tests/test_create_testdataset_submit.py | 160 ++++++++++++++++++++++ 2 files changed, 161 insertions(+), 1 deletion(-) diff --git a/dincli/cli/modelownerd/auditor_batches.py b/dincli/cli/modelownerd/auditor_batches.py index ec7f8ed1..dc9ec072 100644 --- a/dincli/cli/modelownerd/auditor_batches.py +++ b/dincli/cli/modelownerd/auditor_batches.py @@ -297,7 +297,7 @@ def create_testdataset( ) console.print( f"[yellow]Skipping the test-dataset-assigned flag. Retry just the failed " - f"batch(es) with: dincli model-owner task auditor-batches create-testdataset " + f"batch(es) with: dincli model-owner auditor-batches create-testdataset " f"{model_id} --submit {retry_flags}[/yellow]" ) raise typer.Exit(1) diff --git a/tests/test_create_testdataset_submit.py b/tests/test_create_testdataset_submit.py index 76c2f0c1..e01cd672 100644 --- a/tests/test_create_testdataset_submit.py +++ b/tests/test_create_testdataset_submit.py @@ -4,9 +4,14 @@ import pytest import typer from nacl.public import PrivateKey +from typer.testing import CliRunner from web3 import Web3 +from dincli.cli.context import DinContext from dincli.cli.modelownerd import auditor_batches as atb +from dincli.main import app as main_app + +runner = CliRunner() _TA_FLAG_ALREADY_SET_SELECTOR = Web3.keccak(text="TA_FlagAlreadySet()").hex()[:8] @@ -283,3 +288,158 @@ def fake_build_and_send_tx(ctx, contract_function, action_msg, success_msg, erro with pytest.raises(typer.Exit): _invoke(tmp_path, ctx) + + +# --------------------------------------------------------------------------- +# CLI-level coverage (issue #252 approval amendment 5): exercise the +# registered `dincli model-owner auditor-batches create-testdataset` command +# through CliRunner/typer's real argument parsing, not by calling the Python +# function directly. This is what catches a wrong command path in printed +# output (e.g. the retry hint) that a direct-call test can't see. +# --------------------------------------------------------------------------- + +def _patch_context_methods(monkeypatch, tmp_path, coordinator, auditor, stake=None): + stake_contract = stake or DummyStakeContract() + account = DummyAccount() + + monkeypatch.setattr( + DinContext, "get_en_w3_account_console", + lambda self, *a, **kw: ("local", object(), account, self.console), + ) + monkeypatch.setattr( + DinContext, "get_deployed_din_task_coordinator_contract", + lambda self, *a, **kw: coordinator, + ) + monkeypatch.setattr( + DinContext, "get_deployed_din_task_auditor_contract", + lambda self, *a, **kw: auditor, + ) + monkeypatch.setattr( + DinContext, "get_current_gi_and_state", + lambda self, contract, *a, **kw: (contract.gi, contract.gi_state), + ) + monkeypatch.setattr(DinContext, "validate_gi_ET_curr_GI", lambda self, gi, curr_gi: curr_gi) + monkeypatch.setattr(DinContext, "validate_GIstate_ET_given_GIstate", lambda self, *a, **kw: None) + monkeypatch.setattr(DinContext, "get_model_base_dir", lambda self, model_id: tmp_path) + monkeypatch.setattr(DinContext, "ensure_file_exists", lambda self, *a, **kw: None) + monkeypatch.setattr(DinContext, "get_deployed_din_stake_contract", lambda self, *a, **kw: stake_contract) + + def fake_fn(batch_count, gi, model_base_path, test_data_path): + return [ + { + "raw_cid": f"cid-{i}", + "K": secrets.token_bytes(32), + "plaintext_keccak": b"\x00" * 32, + } + for i in range(batch_count) + ] + + monkeypatch.setattr(DinContext, "load_custom_fn", lambda self, *a, **kw: fake_fn) + + +def _make_test_data_file(tmp_path): + test_data_file = tmp_path / "test_dataset.pt" + test_data_file.write_bytes(b"dummy") + return test_data_file + + +def _cli_args(model_id, test_data_file, batch=None, submit=True): + args = ["model-owner", "auditor-batches", "create-testdataset", str(model_id)] + if submit: + args.append("--submit") + args += ["--test-data-path", str(test_data_file)] + for b in batch or []: + args += ["--batch", str(b)] + return args + + +def test_cli_all_batches_succeed_sends_flag(monkeypatch, tmp_path): + coordinator = DummyTaskCoordinatorContract() + auditor = DummyTaskAuditorContract(batch_count=3) + _patch_context_methods(monkeypatch, tmp_path, coordinator, auditor) + + sent = [] + + def fake_build_and_send_tx(ctx, contract_function, action_msg, success_msg, error_msg, *a, **kw): + sent.append(contract_function) + return SimpleNamespace(transactionHash=b"\xab" * 32) + + monkeypatch.setattr(atb, "build_and_send_tx", fake_build_and_send_tx) + + result = runner.invoke(main_app, _cli_args(1, _make_test_data_file(tmp_path))) + + assert result.exit_code == 0, result.output + assign_calls = [c for c in sent if isinstance(c, tuple) and c[0] == "assignAuditTestDataset"] + assert sorted(c[1] for c in assign_calls) == [0, 1, 2] + assert any(not isinstance(c, tuple) for c in sent) + + +def test_cli_one_batch_fails_exits_1_and_prints_correct_retry_command(monkeypatch, tmp_path): + coordinator = DummyTaskCoordinatorContract() + auditor = DummyTaskAuditorContract(batch_count=3) + _patch_context_methods(monkeypatch, tmp_path, coordinator, auditor) + + def fake_build_and_send_tx(ctx, contract_function, action_msg, success_msg, error_msg, *a, **kw): + if isinstance(contract_function, tuple) and contract_function[1] == 1: + return None + return SimpleNamespace(transactionHash=b"\xab" * 32) + + monkeypatch.setattr(atb, "build_and_send_tx", fake_build_and_send_tx) + + result = runner.invoke(main_app, _cli_args(1, _make_test_data_file(tmp_path))) + + assert result.exit_code == 1, result.output + assert "dincli model-owner auditor-batches create-testdataset 1 --submit --batch 1" in result.output + assert "model-owner task auditor-batches" not in result.output + + +def test_cli_batch_retry_only_sends_listed_batches_then_flag(monkeypatch, tmp_path): + coordinator = DummyTaskCoordinatorContract() + auditor = DummyTaskAuditorContract(batch_count=3) + _patch_context_methods(monkeypatch, tmp_path, coordinator, auditor) + + sent = [] + + def fake_build_and_send_tx(ctx, contract_function, action_msg, success_msg, error_msg, *a, **kw): + sent.append(contract_function) + return SimpleNamespace(transactionHash=b"\xab" * 32) + + monkeypatch.setattr(atb, "build_and_send_tx", fake_build_and_send_tx) + + result = runner.invoke(main_app, _cli_args(1, _make_test_data_file(tmp_path), batch=[1])) + + assert result.exit_code == 0, result.output + assign_calls = [c for c in sent if isinstance(c, tuple) and c[0] == "assignAuditTestDataset"] + assert [c[1] for c in assign_calls] == [1] + assert any(not isinstance(c, tuple) for c in sent) + + +def test_cli_flag_already_set_is_informational_exit_0(monkeypatch, tmp_path): + coordinator = DummyTaskCoordinatorContract(flag_call_raises=flag_already_set_revert()) + auditor = DummyTaskAuditorContract(batch_count=2) + _patch_context_methods(monkeypatch, tmp_path, coordinator, auditor) + + def fake_build_and_send_tx(ctx, contract_function, action_msg, success_msg, error_msg, *a, **kw): + return SimpleNamespace(transactionHash=b"\xab" * 32) + + monkeypatch.setattr(atb, "build_and_send_tx", fake_build_and_send_tx) + + result = runner.invoke(main_app, _cli_args(1, _make_test_data_file(tmp_path))) + + assert result.exit_code == 0, result.output + assert "already set" in result.output.lower() + + +def test_cli_flag_other_revert_exits_1(monkeypatch, tmp_path): + coordinator = DummyTaskCoordinatorContract(flag_call_raises=other_revert()) + auditor = DummyTaskAuditorContract(batch_count=2) + _patch_context_methods(monkeypatch, tmp_path, coordinator, auditor) + + def fake_build_and_send_tx(ctx, contract_function, action_msg, success_msg, error_msg, *a, **kw): + return SimpleNamespace(transactionHash=b"\xab" * 32) + + monkeypatch.setattr(atb, "build_and_send_tx", fake_build_and_send_tx) + + result = runner.invoke(main_app, _cli_args(1, _make_test_data_file(tmp_path))) + + assert result.exit_code == 1, result.output From f0f05e3e79015b7094def43cd3f14810ee462b4d Mon Sep 17 00:00:00 2001 From: Abidoyesimze Date: Mon, 26 Oct 2026 21:23:35 +0100 Subject: [PATCH 3/3] fix(dincli): handle richer GI-states lifecycle in create-testdataset rerun (PR 258 review) Addresses the two outstanding items from review: 1. Added a CLI test exercising two distinct repeated --batch options (--batch 0 --batch 2), asserting only those batches are assigned and the flag is then attempted. The existing retry test only covered a single --batch. 2. Once the richer GI-states lifecycle (task_061026_21) is live, setTestDataAssignedFlag succeeding advances GIstate to AuditTestDataAssigned, while create-testdataset's gate only accepted AuditorsBatchesCreated -- so a documented-safe re-run would actually be rejected by the exact-match gate with a generic wrong-state error before ever reaching the already-set handling. Replaced the gate with a check that recognizes AuditTestDataAssigned directly and reports "nothing further to do" without attempting any batch work, and accepts AuditorsBatchesCreated as before. The check reads the state name itself rather than calling the no-op-mockable validate_GIstate_ET_given_GIstate, so the new regression test drives the real logic instead of bypassing it. Verified by temporarily reverting to the old single-state check: the new test fails exactly as expected (proceeds into batch assignment instead of short-circuiting). Also reconciled onto current develop (now past PR #273's revert of the richer GI-states merge), and fixed a pre-existing test (test_create_testdataset_rejects_manifest_path_escape) whose established mock convention passes GIstate as the state name string rather than an int ordinal -- the new gate now accepts either. Co-Authored-By: Claude Sonnet 5 --- dincli/cli/modelownerd/auditor_batches.py | 25 ++++++++- tests/test_create_testdataset_submit.py | 65 ++++++++++++++++++++++- 2 files changed, 87 insertions(+), 3 deletions(-) diff --git a/dincli/cli/modelownerd/auditor_batches.py b/dincli/cli/modelownerd/auditor_batches.py index c567136f..65e77ab5 100644 --- a/dincli/cli/modelownerd/auditor_batches.py +++ b/dincli/cli/modelownerd/auditor_batches.py @@ -11,7 +11,7 @@ from eth_account.messages import encode_defunct from cryptography.hazmat.primitives.ciphers.aead import AESGCM -from dincli.cli.utils import CONFIG_DIR, build_and_send_tx, ensure_batch_seed_locked, get_manifest_key, require_custom_manifest_service, resolve_manifest_path +from dincli.cli.utils import CONFIG_DIR, GIstateToStr, build_and_send_tx, ensure_batch_seed_locked, get_manifest_key, require_custom_manifest_service, resolve_manifest_path from dincli.services.cid_utils import get_bytes32_from_cid, get_cid_from_bytes32 auditor_batches_app = typer.Typer(help="Auditor Batches commands") @@ -170,7 +170,28 @@ def create_testdataset( ref_gi = ctx.obj.validate_gi_ET_curr_GI(gi, curr_GI) - ctx.obj.validate_GIstate_ET_given_GIstate(GIstate, "AuditorsBatchesCreated", "Can not create test dataset at this time") + # Richer GI states (task_061026_21, not always present -- see git history + # around issues #243/#272/#273/#287) advance GIstate to + # AuditTestDataAssigned once setTestDataAssignedFlag succeeds on-chain. + # When that state exists and is current, the flag is already set and + # there is nothing left for this command to do -- treat it the same way + # as the informational "already set" case below, rather than falling + # through to the exact-match gate (which would otherwise reject a safe + # re-run with a generic "wrong state" error, contradicting the + # re-run-safety documented on this command). + curr_GIstate_str = GIstate if isinstance(GIstate, str) else GIstateToStr(GIstate) + if curr_GIstate_str == "AuditTestDataAssigned": + console.print( + "[yellow]Test dataset already assigned and the flag already set for this " + "GI (GIstate is AuditTestDataAssigned) — nothing further to do.[/yellow]" + ) + return + if curr_GIstate_str != "AuditorsBatchesCreated": + console.print( + f"[bold red]✗ Can not create test dataset at this time. " + f"Current state: {curr_GIstate_str} [/bold red]" + ) + raise typer.Exit(1) console.print(f"[bold green]Creating test dataset![/bold green]") diff --git a/tests/test_create_testdataset_submit.py b/tests/test_create_testdataset_submit.py index e01cd672..ae0fb05b 100644 --- a/tests/test_create_testdataset_submit.py +++ b/tests/test_create_testdataset_submit.py @@ -9,8 +9,15 @@ from dincli.cli.context import DinContext from dincli.cli.modelownerd import auditor_batches as atb +from dincli.cli.utils import GIstatestrToIndex from dincli.main import app as main_app +# Resolved dynamically (not hardcoded) so these tests stay correct whether or +# not the richer GI-states lifecycle (task_061026_21) is merged on whatever +# develop snapshot this runs against -- the ordinal for this state name shifts +# depending on that. +_AUDITOR_BATCHES_CREATED_ORDINAL = GIstatestrToIndex("AuditorsBatchesCreated") + runner = CliRunner() @@ -96,7 +103,7 @@ def setTestDataAssignedFlag(self, gi, value): class DummyTaskCoordinatorContract: - def __init__(self, gi=0, gi_state=0, flag_call_raises=None): + def __init__(self, gi=0, gi_state=_AUDITOR_BATCHES_CREATED_ORDINAL, flag_call_raises=None): self.gi = gi self.gi_state = gi_state self.flag_call_raises = flag_call_raises @@ -414,6 +421,62 @@ def fake_build_and_send_tx(ctx, contract_function, action_msg, success_msg, erro assert any(not isinstance(c, tuple) for c in sent) +def test_cli_batch_retry_with_two_distinct_batches_then_flag(monkeypatch, tmp_path): + coordinator = DummyTaskCoordinatorContract() + auditor = DummyTaskAuditorContract(batch_count=3) + _patch_context_methods(monkeypatch, tmp_path, coordinator, auditor) + + sent = [] + + def fake_build_and_send_tx(ctx, contract_function, action_msg, success_msg, error_msg, *a, **kw): + sent.append(contract_function) + return SimpleNamespace(transactionHash=b"\xab" * 32) + + monkeypatch.setattr(atb, "build_and_send_tx", fake_build_and_send_tx) + + result = runner.invoke(main_app, _cli_args(1, _make_test_data_file(tmp_path), batch=[0, 2])) + + assert result.exit_code == 0, result.output + assign_calls = [c for c in sent if isinstance(c, tuple) and c[0] == "assignAuditTestDataset"] + assert [c[1] for c in assign_calls] == [0, 2] + assert any(not isinstance(c, tuple) for c in sent) + + +def test_cli_rerun_after_state_advanced_to_audit_test_data_assigned(monkeypatch, tmp_path): + """Once the richer GI-states lifecycle (task_061026_21) is live, + setTestDataAssignedFlag succeeding advances GIstate to + AuditTestDataAssigned. A safe re-run must recognise that state directly + and report "already done" without touching the exact-match state gate + (which would otherwise reject it with a generic wrong-state error, + contradicting the documented re-run safety) -- and without any batch + creation/assignment/flag work being attempted. This drives + create_testdataset's own dual-state check directly -- that check no + longer calls the generic validate_GIstate_ET_given_GIstate at all for + this command, so it can't be masked by that helper being mocked + elsewhere. Only GIstateToStr itself is substituted, so the scenario is + exercisable regardless of whether this develop snapshot currently has + the richer states merged. + """ + coordinator = DummyTaskCoordinatorContract(gi_state=999) + auditor = DummyTaskAuditorContract(batch_count=3) + _patch_context_methods(monkeypatch, tmp_path, coordinator, auditor) + + monkeypatch.setattr( + atb, "GIstateToStr", + lambda gi_state: "AuditTestDataAssigned" if gi_state == 999 else "AuditorsBatchesCreated", + ) + + def fail_if_called(*a, **kw): + raise AssertionError("must not send any transaction once already past this state") + + monkeypatch.setattr(atb, "build_and_send_tx", fail_if_called) + + result = runner.invoke(main_app, _cli_args(1, _make_test_data_file(tmp_path))) + + assert result.exit_code == 0, result.output + assert "already set" in result.output.lower() or "nothing further to do" in result.output.lower() + + def test_cli_flag_already_set_is_informational_exit_0(monkeypatch, tmp_path): coordinator = DummyTaskCoordinatorContract(flag_call_raises=flag_already_set_revert()) auditor = DummyTaskAuditorContract(batch_count=2)