[bfs2021]

Problem

dincli task model-owner register-request --manifestCID <cid> crashed with
UnboundLocalError: cannot access local variable 'dinregistry_contract' where it is not associated with a value before any transaction was sent (issue #295). The manifest-file
route (--manifestpath) worked.

Root cause

The dinregistry_contract = ctx.obj.get_deployed_din_registry_contract() lookup sat inside
the if not manifestCID: branch, so the CID route skipped it, and the later fee lookup
(openSourceFee() / proprietaryFee()) read an unbound local.
update-manifest-request already resolves the contract before its own branch.

Fix

Move the lookup above the if not manifestCID: branch, mirroring
update-manifest-request. The manifest-file path is unchanged: it still reaches the same
lookup, now resolved once instead of inside the branch.

Testing

• New unit test tests/test_register_request_cid.py: calls register_request with a
  mocked ctx.obj and a --manifestCID argument, asserting the command runs to
  build_and_send_tx with the fee from proprietaryFee(). It fails with
  UnboundLocalError at the fee lookup on unpatched develop and passes with the fix.
• pytest -m "not integration" -q: 432 passed, 1 failed → 18 failed / 12 errors, all
  pre-existing on this Windows machine (identical set on unpatched develop: Unix-only
  permission bits, symlinks, anvil/ipfs/forge tooling; 3 more modules can't collect without
  torch). The new test accounts for the +1 pass; my change adds no failures.
• ruff check dincli/cli/task.py tests/test_register_request_cid.py: the 28 findings in
  task.py are identical on unpatched develop (CI runs ruff as advisory); the new test
  file is clean.

Fixes #295

[umeradl]

Reviewed against develop in an isolated worktree (branch applies cleanly on top of current tip f5b2284; merge-base f2cff58; 2 commits, no conflicts — confirmed by both git merge-tree --write-tree locally and GitHub's own mergeable: MERGEABLE / mergeStateStatus: BEHIND). The only commit develop picked up since the merge-base is f5b2284, which touches Developer/GOOD_FIRST_ISSUES.md only — disjoint from this PR.

CI on d1b6bdc (the code commit): CI OK green — Python, Solidity, Docs, Integration paths all green. The head commit 9ccf090 is an empty commit (0 files changed), so d1b6bdc's tree is the head tree; its CI run is waiting on fork-workflow approval (action_required). Reproduced locally: a break-then-fix check of the new test and the full non-integration suite.

Claimed: register-request --manifestCID <cid> hit an UnboundLocalError at the fee lookup because dinregistry_contract was only assigned inside if not manifestCID:.

Verified — correct: on develop, dincli/cli/task.py assigns dinregistry_contract only inside the if not manifestCID: block, and the CID route next reads it at openSourceFee() / proprietaryFee(). No assignment on that path.

Claimed: moving the lookup above the branch mirrors update-manifest-request, and the manifest-file path is unchanged.

Verified — correct: update_manifest_request calls get_deployed_din_registry_contract() before its own if not manifestCID:. In register_request the lookup now runs once, right after get_en_w3_account_console() and before the task-coordinator/auditor resolution. get_deployed_din_registry_contract() is a read-only contract lookup with no prompts or side effects. Moving it earlier only changes the order of the file-route steps: a missing registry deployment now errors before the manifest-path checks and confirmations instead of after the IPFS upload, which is arguably better (fail before uploading).

Claimed: the new tests/test_register_request_cid.py fails with UnboundLocalError on unpatched develop and passes with the fix.

Verified — break-then-fix: with develop's dincli/cli/task.py swapped into the worktree, pytest tests/test_register_request_cid.py gives 1 failed with UnboundLocalError: cannot access local variable 'dinregistry_contract' at dincli/cli/task.py:251. With the PR's file restored it gives 1 passed. The test also asserts that the proprietaryFee() value is what gets sent as tx_params["value"], so it checks the fee wiring, not just the absence of a crash.

Claimed: no new test failures.

Verified — Linux, full suite: pytest -m "not integration" in the worktree: 540 passed, 133 deselected, 0 failed (the author's Windows-only failures don't occur here). Also covered by the green CI Python job.

Not independently re-verified: a live register-request --manifestCID against anvil/IPFS. The unit test covers the fixed code path end to end up to build_and_send_tx, and the change is a single statement moved. Also unrelated to this PR: the non-blocking Integration (full GI) job failed on d1b6bdc at test_01_platform.py::test_deploy_platform_via_script (missing proxyAdmin in foundry/deployments/localhost.json). The deploy now writes one ProxyAdmin per proxy, and the tests/dincli suite still expects a single proxyAdmin key. That's pre-existing drift in the integration suite, not caused by this diff.

---

Every checkable claim held up. Mergeable as-is; GitHub shows BEHIND, but the only new develop commit is a disjoint docs file, so an update-branch isn't needed for correctness. Once this lands, the --manifestCID route can go back into the model-owner docs (issue No. 295 asked for the docs to show only --manifestpath until it was fixed).

[umeradl]

Files changed (2) — as of 9ccf090 (PR head)

Diffed against merge-base f2cff58 (develop). develop has moved one commit since (f5b2284, Developer/GOOD_FIRST_ISSUES.md only), which overlaps neither of this PR's files. GitHub agrees: mergeable: MERGEABLE, mergeStateStatus: BEHIND. Confirmed with a local git merge-tree --write-tree dry run: clean.

dincli/cli/task.py

Field	Value
Change	Modified
Lines	+2/-2
Diff (what exactly is in this PR)	Moves dinregistry_contract = ctx.obj.get_deployed_din_registry_contract() from the end of the if not manifestCID: block in register_request to just after get_en_w3_account_console(), before the task-coordinator/auditor resolution.
Functionality — how & why	How: register_request now resolves the registry contract unconditionally. Both routes (manifest file → IPFS upload, or a caller-supplied --manifestCID) then reach the same openSourceFee()/proprietaryFee() call, requestModelRegistration(...) tx and ModelRegistrationRequested receipt parse with the contract bound. Why: on the CID route the block that assigned it was skipped, so the fee lookup raised UnboundLocalError before any tx (issue No. 295). This matches update_manifest_request, which already resolved the contract before its branch.
Diff vs current develop HEAD	None — untouched since merge-base.
Recommended merge proposal	Merged as-is. Break-then-fix confirmed: develop's version of this file makes the new test fail with the reported UnboundLocalError; the PR's version passes.
Actual merge proposal	Merged as-is (path B, local merge by @umeradl) in e9424af; no deviation commit.
Pending proposal	None
Local merge conflict	No
GitHub merge conflict	No

tests/test_register_request_cid.py

Field	Value
Change	New
Lines	+65/-0
Diff (what exactly is in this PR)	Regression test: calls register_request directly with a SimpleNamespace ctx.obj (mocked registry contract, w3, account, console that auto-confirms) on the --manifestCID route, with get_bytes32_from_cid and build_and_send_tx patched.
Functionality — how & why	How: asserts get_deployed_din_registry_contract() is called exactly once, build_and_send_tx is reached once, and the tx value equals the mocked proprietaryFee(). Why: locks in the CID route so the lookup can't drift back inside the file-route branch, and checks the fee wiring, not just the absence of a crash.
Diff vs current develop HEAD	None — file doesn't exist on develop.
Recommended merge proposal	Merged as-is. Passes locally (1 passed); full pytest -m "not integration": 540 passed, 0 failed.
Actual merge proposal	Merged as-is (path B, local merge by @umeradl) in e9424af; no deviation commit.
Pending proposal	None
Local merge conflict	No
GitHub merge conflict	No

Verification

CI on d1b6bdc (tree identical to head 9ccf090, which is an empty commit): CI OK green. Locally: break-then-fix of the new test, plus a full pytest -m "not integration" run, 540 passed. Full detail is in the verification comment above. The non-blocking Integration (full GI) failure (missing proxyAdmin) is pre-existing tests/dincli drift, unrelated to this diff.

Local vs. GitHub agree: both clean; GitHub's BEHIND reflects only a disjoint docs commit on develop.

[umeradl]

Verified at 9ccf090 (tree = d1b6bdc): break-then-fix of the new test, 540 unit tests pass, CI OK green.

[umeradl]

Actual outcome — PR No. 297 merged as-is

This supersedes the pre-merge review comment above with what actually happened when merging this PR into develop.

One commit, on origin/develop:

1. e9424af: real merge of this PR (a merge commit, not a squash; parents 3e738c4 and this PR's head 9ccf090). Path B, a local merge by @umeradl after his approval of 9ccf090. The admin bypass was used because the branch was BEHIND develop on unrelated drift and the fork CI run for the empty re-trigger head was still awaiting approval; it's logged in Discussion 170. 0 conflicts, as predicted. GitHub agrees: the PR shows MERGED. No deviation commit was needed.

Files unchanged from the PR (2 of 2)

• dincli/cli/task.py
• tests/test_register_request_cid.py

develop had moved four commits since the review (f5b2284 → 3e738c4: the DP-labelling merge of PR No. 288 plus docs/README updates). None of them touches either file, and the merged tree needed no adaptation.

Verification

• Baseline on develop 3e738c4 before the merge: forge test 516 passed / 0 failed; pytest -m "not integration" 542 passed.
• Merged tree: pytest -m "not integration" 543 passed (+1 = the new test), 0 failed. Solidity is untouched by this PR.
• Local CI mirror on e9424af (docs links, pytest with an empty HOME): OK.
• develop push run on e9424af: CI green.

Local vs. GitHub agree: clean merge as predicted. Issue No. 295 can now be closed, and the --manifestCID route can go back into the model-owner docs.

[umeradl]

Note on the outcome comment above: "CI green" refers to CI OK. The non-blocking Integration (full GI) job failed on e9424af at the missing proxyAdmin harness assertion mentioned in the verification comment. That's a known, accepted failure that predates this PR: the suite is being reworked in task_061026_21 Part D. The job is becoming a manual-only workflow in PR No. 301. Unrelated to this diff.