Skip to content

feat: --custody managed + local SDK pin for ENGN-8646 testing - #34

Draft
TaniBuilds wants to merge 12 commits into
mainfrom
tani/engn-8646-managed-custody
Draft

feat: --custody managed + local SDK pin for ENGN-8646 testing#34
TaniBuilds wants to merge 12 commits into
mainfrom
tani/engn-8646-managed-custody

Conversation

@TaniBuilds

@TaniBuilds TaniBuilds commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Enables running a worker with Privy-managed custody (auto-provision a wallet bound to its topic) and wires the builder-kit to a local build of the allora-sdk branch so it can be tested before the SDK is released.

Linear: ENGN-8646.

  • worker_runtime.py: adds --custody {local,managed}. managed builds AlloraWalletConfig.from_env() (FORGE_API_KEY → the SDK provisions a wallet bound to the topic at startup; no local key file). Also migrates to the branch SDK's worker API — AlloraWorker.inferer(...) and a RunContext-based run fn (raw_fn(ctx.nonce)) — since the branch is a newer worker-API generation than the published allora-sdk 1.0.6.
  • pyproject.toml: a clearly-marked local-dev [tool.uv.sources] pin of allora-sdk to the sibling allora-sdk-py worktree (which carries the unreleased auto-provision changes + generated protobuf code).

Not for merge as-is

The [tool.uv.sources] pin and the worker-API migration assume the unreleased SDK. Remove the pin and bump the allora-sdk version once the SDK release with ENGN-8646 ships. This branch is for local testing in the meantime.

Test plan

  • py_compile clean
  • uv run … worker_runtime --help imports against the local SDK and shows --custody {local,managed}
  • Live: with a running forge-v2 backend (Privy auth key + owner configured), FORGE_API_KEY set, run --custody managed --topic N and confirm the worker provisions a managed wallet, registers, and submits (gasless when FEE_GRANTER = master address)

Summary by cubic

Adds a --custody managed mode to run workers with Privy-managed wallets that auto-provision per topic (ENGN-8646), and migrates the builder kit to the branch allora-sdk RunContext-based worker API. Keeps a local-dev pin to the allora-sdk branch for testing until the release ships.

  • New Features

    • worker_runtime.py: --custody {local,managed}; managed uses AlloraWalletConfig.from_env() with FORGE_API_KEY to provision a wallet bound to topic_id (no local key).
    • Migrates to AlloraWorker.inferer(...) and a RunContext-based run fn; raw notebook updated to match.
  • Bug Fixes

    • Resolve wallet config in sync main() via _resolve_wallet_cfg to avoid blocking the event loop; pass pre-resolved wallet_cfg into _run().
    • Prevent silent submission drops by detecting artifact call-shape; support both fn(nonce) and fn(ctx).
    • CLI guards: require FORGE_API_KEY for managed, reject --mnemonic-file with managed, and warn when FEE_GRANTER is unset.

Written for commit 7e0e80d. Summary will update on new commits.

Review in cubic

Lets a worker run with Privy-managed custody (auto-provision a wallet bound to
its topic) and migrates worker_runtime to the branch SDK's worker API:

- worker_runtime.py: add --custody {local,managed}; managed builds
  AlloraWalletConfig.from_env() (FORGE_API_KEY → provision per-topic, no local
  key). Migrate to AlloraWorker.inferer(...) and adapt the run fn to the
  RunContext-based signature (raw_fn(ctx.nonce)).
- pyproject.toml: LOCAL-DEV [tool.uv.sources] pin of allora-sdk to the local
  branch worktree (carries the unreleased auto-provision changes). Remove and
  bump the version once the SDK release ships.
@TaniBuilds

TaniBuilds commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

🔍 Multi-Agent Multi-Repo Code Review 🧑‍💻

16 findings across 9 dimensions — 🟡 4 medium · 🔵 7 low · ⚪ 5 info

🧵 Synthesized by anthropic/claude-sonnet-4.6 — 31 raw findings → 16 clusters (7 dismissed)

Dimensions: adversarial architecture correctness cross-repo deep-analysis performance runtime-semantics security style · Models: anthropic/claude-sonnet-4.6


Correctness (6 findings)

🟡 synth-001 (medium) — allora_forge_builder_kit/worker_runtime.py:145
--custody local default + no --mnemonic-file → SDK getpass() hangs headless workers or silently creates an unfunded wallet

When --custody local (the default) is used without --mnemonic-file, _resolve_wallet_cfg('local', None) returns None. The ENGN-8646 SDK's init_worker_wallet(None, topic_id) falls through to: (a) read .allora_key from CWD if present, OR (b) call getpass('Mnemonic: ') for interactive input, AND if the operator presses Enter, silently generates a fresh mnemonic and writes it to .allora_key in CWD.

Consequences:

  1. In headless/supervised mode (WorkerManager, systemd, Docker without TTY), the worker hangs indefinitely on getpass. Operators see a worker process that never starts producing submissions.
  2. If getpass succeeds with empty input, the SDK creates a brand-new, unfunded wallet the operator did not consciously create. Combined with --no-faucet, the worker signs with a zero-balance wallet and every tx fails.
  3. The .allora_key CWD path is easy to confuse with the kit's _load_api_key fallback paths.

The three --custody managed guards in main() use parser.error() for strict pre-flight validation; the --custody local path has no symmetric guard. No Phase-A agent flagged this — the adversarial reviewer caught it by inspecting the ENGN-8646 SDK branch directly (utils.py lines 47-64).

💡 Add a guard in main() for --custody local that errors when neither --mnemonic-file nor ./.allora_key exists, mirroring the managed-mode guards.

🔗 Cross-repo: affects allora-sdk-py

🔵 synth-005 (low) — allora_forge_builder_kit/worker_runtime.py:76
_artifact_expects_context has three distinct misclassification modes: name heuristic, substring annotation match, and class artifacts

🤝 flagged by 3 agents: correctness, deep-analysis, adversarial

Three failure modes in _artifact_expects_context (lines 76-95):

1. Name-based dispatch false-positives (correctness-2, DA-001): Parameters named ctx, context, or run_context trigger RunContext dispatch regardless of type. A legacy artifact def predict(ctx): return float(ctx) * scale — where ctx is the author's name for the integer nonce — will receive a RunContext object and raise TypeError / AttributeError on every inference cycle. This appears as silent topic non-participation because the SDK's Inferer catches and logs the error per submission without raising. The universe of pre-existing artifacts using these param names is small but non-zero (cloudpickle preserves source names verbatim).

2. Substring annotation match (adv-3): "RunContext" in annotation matches MyRunContext, FastRunContextWrapper, WorkerRunContextV2, and any user-defined class containing "RunContext" as a substring. Such artifacts would receive an SDK RunContext instance instead of their custom type. Fix: exact-token match on the trailing qualified-name component.

3. Class-based artifacts (adv-6): inspect.signature on a class returns the __init__ signature, not __call__. A pickled class whose __init__ takes a param named ctx would trigger false-positive RunContext dispatch — constructing a new instance per nonce instead of calling the intended prediction entry point. Tests do not cover the class case.

🎭 adversarial note: Phase B (adv-1) challenged DA-001's HIGH severity as inflated (matching correctness-2's LOW) and warned that DA-001's proposed execution-based probe would replace a safe static check with a side-effect-prone runtime probe of untrusted cloudpickle-deserialized code — double-executing non-idempotent artifacts and introducing broad TypeError/AttributeError catch-all logic. The current static inspect.signature approach is architecturally correct; only the matching logic needs tightening. Severity kept at LOW.

💡 Apply three targeted fixes: (1) Tighten the annotation match from substring to exact-token: annotation.strip("'\"").rsplit(".", 1)[-1] == "RunContext". (2) Handle class artifacts by inspecting __call__ when inspect.isclass(raw_fn). (3) For the name-based heuristic, document its accepted names in the docstring (see synth-014) and note the false-positive risk for legacy ctx-named params — optionally require annotation to take precedence over name alone.

🔵 synth-006 (low) — notebooks/deploy_worker_raw.py:54
Notebook deploy_worker_raw.py hardcodes legacy nonce adapter — breaks for RunContext-style artifacts and diverges from worker_runtime.py

🤝 flagged by 3 agents: correctness, architecture, style

The updated docstring for notebooks/deploy_worker_raw.py (line 12-13) advertises "Targets the branch allora_sdk worker API (AlloraWorker.inferer + a RunContext-based run fn), matching worker_runtime.py." However, _run_fn at line 54 is hardcoded as def _run_fn(ctx): return predict_fn(ctx.nonce). If a user builds an artifact to the new SDK contract — e.g., def predict(ctx: RunContext): use ctx.client.emissions.get_topic(ctx.topic_id) — this adapter passes ctx.nonce (an int) as the argument, and the first inference cycle fails with AttributeError: 'int' object has no attribute 'client' (or similar). The error is caught per-submission by the SDK, producing silent topic non-participation.

The companion worker_runtime.py solved this via _artifact_expects_context dual-mode detection; the notebook bypasses that entirely, creating a divergent adapter. Operators who copy the notebook pattern for new RunContext-style artifacts will see unexpected failures.

Additionally, _run_fn has no type annotation on ctx and no return type — inconsistent with worker_runtime.py's correctly annotated run_fn(ctx: RunContext) -> float:.

💡 Either (a) export a wrap_run_fn(raw_fn) helper from allora_forge_builder_kit.worker_runtime that performs the signature dispatch and use it in the notebook; (b) import and call _artifact_expects_context directly; or (c) revert the docstring to say this script targets legacy fn(nonce) artifacts only and add a note pointing users to worker_runtime.py for the dual-mode dispatch. Also add def _run_fn(ctx: "RunContext") -> float: annotation for consistency.

🔵 synth-007 (low) — allora_forge_builder_kit/worker_runtime.py:176
FEE_GRANTER absent warning uses warnings.warn — suppressible by parent process, inconsistent with adjacent parser.error guards

🤝 flagged by 2 agents: correctness, style

When --custody managed is used without FEE_GRANTER set, main() emits the requirement via warnings.warn(...) (lines 176-181). UserWarning is subject to Python's warning filter machinery: if the WorkerManager (named in the docstring as the typical invoker) or any parent process sets PYTHONWARNINGS=ignore, runs with -W ignore, or calls warnings.simplefilter('ignore'), this warning vanishes silently. The operator then gets no indication that gasless submission will fail with 'insufficient fees' — potentially hours after startup on sparse topic windows.

The two adjacent managed-custody guards (lines 164-168 and 170-174) use parser.error() (hard fail at startup); this third guard is qualitatively inconsistent. Additionally, the warnings.warn call omits the explicit category=UserWarning keyword argument, making warning-filter configuration less discoverable.

💡 Either escalate to parser.error(...) if FEE_GRANTER is genuinely required for the managed path, or write unconditionally to sys.stderr to bypass filters. At minimum, add category=UserWarning for explicitness.

🔵 synth-009 (low) — tests/test_worker_runtime.py:104
Dispatch wiring in run_fn closure is untested — only the _artifact_expects_context heuristic is covered

Five tests in tests/test_worker_runtime.py (lines 104-133) cover _artifact_expects_context in isolation, but the actual dispatch — the run_fn(ctx) closure (worker_runtime.py lines 118-130) that calls raw_fn(ctx) vs raw_fn(ctx.nonce) based on expects_context — is never exercised. A typo swapping the branch condition, a future refactor dropping the if expects_context check, or a copy-paste error could silently invert the dispatch without failing any test.

Given the entire PR's correctness rests on this dispatch being wired correctly to the SDK's RunContext-callback contract (cross-repo verified), the missing coverage is a notable gap. The heuristic tests give confidence that _artifact_expects_context returns the right bool, but do not regression-protect the run_fn wiring itself.

💡 Extract the dispatch from _run into a small _build_run_fn(raw_fn, reject_zero) -> Callable[[RunContext], float] helper and add two unit tests: one verifying that a legacy def f(nonce): ... artifact is invoked with ctx.nonce, and one verifying that a def f(ctx): ... artifact is invoked with the ctx object. Use types.SimpleNamespace(nonce=42) as a RunContext stub.

⚪ synth-012 (info) — allora_forge_builder_kit/worker_runtime.py:149
--api-key argument missing help text — invites confusion between ALLORA_API_KEY and FORGE_API_KEY under --custody managed

parser.add_argument("--api-key", default=None) has no help= string (line 149). The module docstring carefully distinguishes ALLORA_API_KEY (consumer/faucet) from FORGE_API_KEY (Forge backend, managed wallet), but --help shows --api-key API_KEY with no description. An operator running --custody managed who sees an "ALLORA_API_KEY not found" error from _load_api_key may incorrectly supply their FORGE_API_KEY via --api-key, causing a 401 from the topic-query API or leaking the Forge key in shell history.

💡 Add: help="Allora consumer API key (ALLORA_API_KEY) for faucet/topic queries — distinct from FORGE_API_KEY used by --custody managed"

🔒 Security (2 findings)

🟡 synth-003 (medium) — allora_forge_builder_kit/worker_runtime.py:58
Kit lacks defensive guard rejecting PRIVATE_KEY/MNEMONIC/MNEMONIC_FILE under managed custody — wrong-SDK-branch resolution causes silent credential confusion

🤝 flagged by 2 agents: security, adversarial

_resolve_wallet_cfg documents "FORGE_API_KEY always takes precedence — there is no silent local-key fallback." This claim is correct ONLY on the tani/engn-8646-auto-provision-managed-wallets branch (confirmed by cross-repo agent via direct inspection). However, the [tool.uv.sources] pin is a CWD-relative path with no branch or SHA constraint. If a reviewer, CI machine, or downstream consumer has the wrong SDK branch checked out (tani/ENGN-8615-sdk-privy-delegated-signing or any released SDK without ENGN-8646), AlloraWalletConfig.from_env() falls through to read PRIVATE_KEY/MNEMONIC/MNEMONIC_FILE. An operator running --custody managed with stale local-key env vars would silently sign transactions with their local key while believing they are using Privy-managed custody.

The kit's only managed-custody guard is FORGE_API_KEY presence (line 170-174); it does NOT detect or reject conflicting local-key env vars. The --mnemonic-file mutual-exclusion guard catches the explicit flag but not the env vars that from_env() reads.

🎭 adversarial note: Phase B (adv-5) challenged sec-001's HIGH severity: cross-repo agent verified the code IS correct against the actual pinned ENGN-8646 branch — 'FORGE_API_KEY always takes precedence' is accurate there. The threat model driving HIGH severity is 'wrong SDK branch resolved by the path pin', which is a build-config gap, not a code bug. Treated as defense-in-depth at MEDIUM. The primary remediation should be replacing the relative path pin with a git+SHA reference (synth-002/synth-015); the kit-side env-var rejection guard is belt-and-suspenders.

💡 Add a defensive guard in _resolve_wallet_cfg or main() that rejects PRIVATE_KEY, MNEMONIC, MNEMONIC_FILE env vars under managed custody with a hard error, so the kit fails closed regardless of which SDK branch resolves the pin.

🔗 Cross-repo: affects allora-sdk-py

🟡 synth-004 (medium) — allora_forge_builder_kit/worker_runtime.py:31
_load_api_key falls back to CWD-relative key files with no ownership check

_load_api_key (lines 31-41) falls back to os.path.exists / open on "notebooks/.allora_api_key" and ".allora_api_key" — both CWD-relative with no canonicalization or file-ownership check. A worker started in an attacker-writable scratch directory silently reads an attacker-supplied consumer key, causing the worker to authenticate to the Allora consumer/faucet API under an attacker-chosen identity (polluting attribution, draining the attacker's faucet quota, or confusing inference records). Under --custody managed, where the operator may have deliberately avoided local key material, this CWD fallback still loads an implicit key from disk without any indication.

This is a pre-existing pattern not introduced by this PR, but is now load-bearing for the new managed-custody flow since ALLORA_API_KEY is still required even under --custody managed.

💡 Resolve the key-file path against the kit's install location (os.path.dirname(__file__)) rather than CWD, and verify the file is not world-writable (os.stat mode check) before reading. Or remove the file fallback entirely and require ALLORA_API_KEY / --api-key explicitly.

🏗️ Architecture (2 findings)

🟡 synth-002 (medium) — pyproject.toml:25
allora-sdk>=1.0.6 dependency floor does not gate on AlloraWorker.inferer — clean pip install . fails at runtime

The PR switches from AlloraWorker(run=run_fn, ...) to AlloraWorker.inferer(run=run_fn, ...) (worker_runtime.py line 133, notebooks/deploy_worker_raw.py line 64). The .inferer() classmethod factory exists on the sibling tani/engn-8646-auto-provision-managed-wallets branch but is absent from the published allora-sdk==1.0.6 release on PyPI.

The [tool.uv.sources] block hides this for the author's machine (uv resolves the local worktree), but a clean pip install . — which ignores [tool.uv.sources] — resolves allora-sdk==1.0.6 from PyPI and worker_runtime.py fails at the first worker construction with AttributeError: type object 'AlloraWorker' has no attribute 'inferer'. Any downstream user, GitHub Actions step using pip, or wheel build is affected. The declared dependency floor is therefore wrong for the actual API surface the kit now uses.

💡 Bump the allora-sdk dependency floor in [project].dependencies to the first release that ships AlloraWorker.inferer and the deferred-managed from_env() path. Until that release exists, mark the kit version as a pre-release and document in the README that uv + the sibling worktree pin is mandatory for installation. Add a CI matrix step that installs via plain pip install . (no uv, no local path) to catch floor regressions.

🔗 Cross-repo: affects allora-sdk-py

⚪ synth-016 (info) — allora_forge_builder_kit/worker_runtime.py:76
Signature dispatch reverse-engineers SDK call conventions by string-matching instead of consuming a stable SDK adapter

_artifact_expects_context (lines 76-95) decides how to invoke the pickled artifact by string-matching parameter names and annotation strings — reverse-engineering the SDK's TRunFn = Callable[[RunContext], ...] contract. This is fragile against: (a) the SDK renaming RunContext; (b) alternate context types the SDK may add (e.g., ReputerContext already exists in reputer.py:31); (c) artifacts whose param is named ctx but expects a different type.

Architecturally, the legacy-nonce→RunContext adapter belongs in the SDK (which owns the RunContext type and the TRunFn contract) as a documented wrap_legacy_nonce_fn / from_legacy_nonce_fn helper, not in a downstream kit doing reflective introspection on cloudpickled callables. The SDK already has make_reputer_function in worker/utils.py as precedent. The kit has no compile-time guarantee that its dispatch matches what inferer.submit actually passes.

💡 Open a follow-up on allora-sdk-py to expose a allora_sdk.worker.utils.legacy_nonce_fn(fn: Callable[[int], float]) -> Callable[[RunContext], float] adapter. The kit would call it for legacy artifacts and pass RunContext-typed artifacts through unchanged, delegating contract ownership to the SDK.

🔗 Cross-repo: affects allora-sdk-py

🎨 Style (4 findings)

🔵 synth-008 (low) — allora_forge_builder_kit/worker_runtime.py:164
Triple-repeated args.custody == "managed" guard should be collapsed into a single block

main() repeats args.custody == "managed" on three consecutive guard lines (164, 170, 176). The Allora Python style guide requires guard clauses to be minimal and non-repetitive. All three checks are logically cohesive and belong in a single if args.custody == "managed": block. As written, adding a fourth managed-only check risks accidentally omitting the predicate, introducing a regression.

💡 Collapse into a single block:

if args.custody == "managed":
    if args.mnemonic_file:
        parser.error(...)
    if not os.environ.get("FORGE_API_KEY"):
        parser.error(...)
    if not os.environ.get("FEE_GRANTER"):
        warnings.warn(..., category=UserWarning, stacklevel=2)

🔵 synth-010 (low) — tests/test_worker_runtime.py:32
Test helper _FakeWalletConfig.last_init_kwargs uses unparameterized dict; from_env missing return annotation

In tests/test_worker_runtime.py line 32, the class attribute last_init_kwargs: dict | None uses a bare, unparameterized dict. Per the Allora Python style guide, strong typing is required everywhere and bare dict is treated as dict[Any, Any]. Given the fake only ever receives mnemonic_file=... kwargs, dict[str, object] | None is the correct, explicit type. Also, from_env at line 40 has no return type annotation (implicit Any), which is similarly loose — it should be -> object (or the sentinel's type) to match the constraint.

💡 Tighten annotations:

last_init_kwargs: dict[str, object] | None = None

@classmethod
def from_env(cls) -> object:
    cls.from_env_calls += 1
    return cls.from_env_sentinel

⚪ synth-013 (info) — allora_forge_builder_kit/worker_runtime.py:150
--mnemonic-file help text is stale with respect to the new --custody option

--mnemonic-file help says "Path to wallet key file (managed by WorkerManager)" (line 150). With --custody local now exposed as the default, this flag is the user-facing way to point a self-custodial worker at a key file — not exclusively a WorkerManager-internal concern. The word "managed" also conflicts with --custody managed (Privy-managed wallets vs WorkerManager-managed), and the new error message says --mnemonic-file is incompatible with --custody managed, which confuses the two senses of "managed".

💡 Update help to: "Path to wallet mnemonic file (used with --custody local; mutually exclusive with --custody managed)"

⚪ synth-014 (info) — allora_forge_builder_kit/worker_runtime.py:76
_artifact_expects_context docstring does not enumerate the accepted context-indicator parameter names

The docstring (lines 77-83) says the function switches when the parameter is "named like a context" but never lists the accepted names. The actual implementation accepts exactly ("ctx", "context", "run_context") (line 95). context in particular is extremely common in Python (SSL context, logging context, Flask/Django request context, etc.). Without listing these names, developers building new artifacts cannot predict which parameter names will trigger RunContext dispatch — the misclassification risk documented in synth-005 is invisible from the public docstring.

💡 Update the docstring to say: "named exactly as one of: ctx, context, or run_context" and note that any other name (e.g. nonce, value, i) keeps the legacy fn(nonce: int) dispatch.

🔗 Cross Repo (2 findings)

🔵 synth-011 (low) — allora_forge_builder_kit/worker_runtime.py:55
Go and TypeScript SDKs lack a managed-wallet auto-provision equivalent — parity gap for future multi-language workers

The ENGN-8646 deferred-managed custody feature (from_env() deferred config, init_worker_wallet(wallet, topic_id) auto-provision, ForgeBackendClient.provision_wallet/clear_association) exists only on allora-sdk-py@tani/engn-8646-auto-provision-managed-wallets. The captured sibling diffs for allora-sdk-go and allora-sdk-ts (both on tani/ENGN-8615-sdk-privy-delegated-signing) implement Privy-delegated signing for a KNOWN wallet_id but have no equivalent auto-provision surface — no ProvisionWallet, initWorkerWallet, or topic-bound get-or-create.

Not a blocker for this PR (the builder-kit only consumes the Python SDK), but if the product later needs Go or TS workers with managed custody and no pre-registered wallet_id, ENGN-8646 equivalent work would need to land on allora-sdk-go and allora-sdk-ts as well.

💡 Track as a follow-up ticket: if managed custody is needed for Go/TS workers, port the provision_wallet / clear_association / deferred-managed from_env() surface to allora-sdk-go and allora-sdk-ts. No action needed for this PR.

🔗 Cross-repo: affects allora-sdk-go, allora-sdk-ts, allora-sdk-py

⚪ synth-015 (info) — pyproject.toml:52
Cross-repo sibling manifest captured the wrong allora-sdk-py branch — causes false-positive reviews

🤝 flagged by 2 agents: cross-repo, architecture

The sibling manifest matched allora-sdk-py@tani/ENGN-8615-sdk-privy-delegated-signing, and the captured diff is that branch's diff. The PR's [tool.uv.sources] pin resolves allora-sdk to ../allora-sdk-py, whose worktree carries tani/engn-8646-auto-provision-managed-wallets (allora-sdk-py PR #81) — a superset with the deferred-managed from_env() path, provision_wallet, clear_association, and AlloraWorker.inferer.

The cross-repo agent verified directly against the ENGN-8646 branch that all builder-kit contract surfaces are consistent (from_env() deferred-managed path, RunContext.nonce, init_worker_wallet with topic_id, etc.). The prior pass's synth-001 CRITICAL and synth-011 LOW findings were false positives caused by this manifest mismatch.

Until the manifest is updated, every automated review of this PR will produce the same false positives from agents that inspect the wrong sibling branch.

💡 Update the cross-repo sibling manifest to include allora-sdk-py@tani/engn-8646-auto-provision-managed-wallets as the primary sibling (or in addition to ENGN-8615). Add a comment in the [tool.uv.sources] block naming the exact branch: # branch: tani/engn-8646-auto-provision-managed-wallets (allora-sdk-py PR #81).

🔗 Cross-repo: affects allora-sdk-py

📝 Agent Summaries

🎭 adversarial: Six adversarial findings: two challenges to Phase A (DA-001 HIGH severity is inflated and its suggested fix introduces execution-side-effect bugs not present in the current code; sec-001 HIGH conflates a code-contract claim with a build-configuration gap, where the code is actually correct against the documented pinned branch), and four issues Phase A missed. The largest gap is adv-2: the new --custody local default with no --mnemonic-file silently delegates to the SDK's init_worker_wallet, which prompts via interactive getpass() or generates a fresh, unfunded mnemonic to disk — a guard symmetric with the managed-mode parser.error guards is missing. adv-3 tightens a permissive substring match ('RunContext' in annotation matches MyRunContext/FastRunContextWrapper). adv-4 notes that the dispatch closure is untested in isolation (only the heuristic is). adv-6 covers a structural class-vs-function asymmetry in the introspection. Net: the PR remains in good shape for a clearly-marked draft; the highest-leverage missed item is the --custody local guard symmetry.

🏗️ architecture: The PR's operational/correctness surface was thoroughly covered by the prior pass (blocking I/O moved to sync _resolve_wallet_cfg, parser.error guards for managed+mnemonic and missing FORGE_API_KEY, signature dispatch added, Literal typing). From an architecture lens, the remaining gaps are cross-repo contract integrity and SDK-boundary coupling: (1) the kit's --custody managed happy path depends on an SDK branch (engn-8646, PR #81) that is absent from the sibling manifest — the manifest matched the older ENGN-8615 branch whose from_env() requires both FORGE_API_KEY and FORGE_SIGNING_WALLET_ID, making the contract unverifiable to reviewers without the author's local pin; (2) the allora-sdk>=1.0.6 dependency floor is wrong for the newly-called AlloraWorker.inferer factory, which is unreleased — a clean pip install . will fail; (3) the legacy-nonce→RunContext adapter is duplicated with different strategies between worker_runtime.py and deploy_worker_raw.py; (4) the signature dispatch reverse-engineers SDK internals by string-matching rather than consuming a stable SDK adapter. None block merge for a clearly-marked local-dev draft, but arch-001 and arch-002 should be resolved before the pin is removed and the kit is shipped.

✅ correctness: Prior review thoroughly covered the major correctness risks (managed-custody happy-path / SDK pin / blocking from_env in async / dual-mode artifact dispatch / Literal typing) and the author's commits 07c525c, ff77d4a, e5fd3e4, b933c1d, da015ff, d391b1d address them. Remaining issues are residual / smaller: (1) the FEE_GRANTER notification uses warnings.warn rather than a hard parser.error, which is inconsistent with the two adjacent managed-custody guards and can be silently suppressed by parent-process warning filters; (2) the _artifact_expects_context name-based heuristic can misclassify legacy artifacts whose param happens to be named ctx/context/run_context and surface as a per-cycle runtime error rather than a startup error; (3) the updated notebooks/deploy_worker_raw.py docstring advertises RunContext support but the adapter still hardcodes legacy predict_fn(ctx.nonce), contradicting the documented contract; plus two minor CLI-help docs nits (--api-key missing help text invites ALLORA_API_KEY/FORGE_API_KEY confusion under managed custody; --mnemonic-file help is stale).

🔗 cross-repo: Cross-repo verification against the ACTUALLY pinned SDK branch (tani/engn-8646-auto-provision-managed-wallets, allora-sdk-py PR #81) confirms the builder-kit's managed-custody usage is contract-consistent: from_env() deferred-managed path, AlloraWorker.inferer() with topic_id for init_worker_wallet provisioning, RunContext.nonce, and ForgeBackendClient.provision_wallet/clear_association all match. No contract breaks. The prior synth-001 CRITICAL and synth-011 LOW findings were false positives caused by the sibling manifest capturing the wrong branch (ENGN-8615 instead of ENGN-8646) — the manifest gap is the real issue (xrepo-001, medium). Secondary findings: ENGN-8646 managed custody is Python-SDK-only (Go/TS lack an equivalent — parity gap to track, xrepo-003, low); the [tool.uv.sources] path pin silently resolves to the wrong SDK if the sibling worktree is on the wrong branch, with no automated check (xrepo-004, low). The ADR-036 wallet-link device flow (contract A) is untouched.

🔬 deep-analysis: PR improves managed custody wiring and SDK alignment, but the artifact call-shape heuristic introduces a backwards-compatibility regression risk for legacy single-argument functions with context-like parameter naming.

⚡ performance: Performance review of PR #34 (feat: --custody managed + local SDK pin for ENGN-8646 testing). The diff is small and the per-cycle hot path (the inner run_fn closure driving worker.run()) is effectively unchanged: call-shape detection is cached once at load time, and the loop body is a float cast + finite check. The previously-flagged blocking from_env() HTTPS call has been correctly moved out of async _run() into sync main() before asyncio.run(), so it no longer stalls the event loop. The only new per-cycle overhead introduced by this repo's diff is the switch from AlloraWorker(...) to AlloraWorker.inferer(...), which enables the SDK's default SanityCheck and adds a per-submission consensus-fetch RPC; this is SDK-default behavior already documented under the prior correctness finding synth-013, flagged here only for the performance dimension. All other new work is startup-only. No critical, high, or medium performance defects introduced by this repo's diff; remaining perf-relevant surface (managed-wallet get-or-create memoization, per-nonce RPC behavior) is owned by the sibling allora-sdk-py branch.

🔄 runtime-semantics: No runtime-semantics defects identified in this diff; custody routing, callback adaptation, and async worker invocation appear consistent with intended behavior.

🔒 security: Three security findings. The primary one (sec-001, HIGH) re-opens the credential-confusion concern that the author previously dismissed as a false positive: the 'no silent local-key fallback' docstring claim only holds on the sibling allora-sdk-py ENGN-8646 branch, but the [tool.uv.sources] pin is a relative path with no branch/commit pin, so the resolved SDK is non-deterministic. On the cross-repo-manifest-matched sibling (tani/ENGN-8615-sdk-privy-delegated-signing) or any released SDK without ENGN-8646, an operator running --custody managed with stale PRIVATE_KEY/MNEMONIC/MNEMONIC_FILE env vars silently signs with a LOCAL wallet while believing they are using Privy-managed custody. The kit's only managed-custody guard is FORGE_API_KEY presence; it does not reject the conflicting local-key env vars. Fix: fail closed by rejecting those env vars under managed custody in the kit itself, independent of SDK branch. sec-002 (MEDIUM) and sec-003 (INFO) cover the CWD-relative API-key file read and a reminder to mirror the notebook's cloudpickle security note in worker_runtime.py. No injection, authn/authz, SSRF, or crypto misuse issues found in the kit's own diff; the SSRF surface (FORGE_BACKEND_URL) and response-size caps live in the sibling SDK and are reasonable.

🎨 style: The PR introduces --custody managed support cleanly: _resolve_wallet_cfg uses Literal["local", "managed"], run_fn is properly type-annotated, and the new _artifact_expects_context helper is well-designed with a docstring. Style issues that were flagged in prior review passes (synth-008, synth-009) are resolved in the PR head. Remaining style gaps are all low/info severity: (1) the three args.custody == "managed" guards in main() should be collapsed into one block per the guard-clause convention; (2) _run_fn in the notebook equivalent is missing type annotations that the production counterpart correctly has; (3) a bare dict type in the test helper; (4) minor docstring and import-hygiene issues. No critical style violations are present.

📋 synthesis: Pass 1 synthesis for PR #34 (--custody managed + local SDK pin for ENGN-8646 testing). The prior-state shows synth-001 through synth-015 were already posted in a prior pass; the author addressed the CRITICAL (confirmed false positive — wrong sibling branch in manifest) and acknowledged the HIGH (uv.sources pin intentional for draft). Several prior issues are now fixed in the PR HEAD: blocking from_env() moved to sync main(), parser.error guards for managed+mnemonic and missing FORGE_API_KEY added, _artifact_expects_context detection added, run_fn properly typed, Literal typing used in _resolve_wallet_cfg.

New findings in this pass (not in prior state): The most important missed issue is synth-001 (MEDIUM): the --custody local default with no --mnemonic-file silently delegates to the SDK's getpass()/mnemonic-generate path — hanging headless workers or creating unfunded wallets — while the symmetric managed-mode guards all use hard parser.error(). synth-002 (MEDIUM) notes the allora-sdk>=1.0.6 dependency floor is wrong for AlloraWorker.inferer which doesn't exist in PyPI's 1.0.6. synth-003 (MEDIUM, challenged from HIGH) is a defense-in-depth guard against wrong-SDK-branch credential confusion. synth-005 (LOW, 4-agent cluster) identifies three distinct misclassification modes in _artifact_expects_context: name-based heuristic collisions, substring annotation match, and class artifacts — the adversarial reviewer specifically warned against the execution-probe fix proposed by deep-analysis. synth-006 (LOW) flags the notebook adapter hardcoding the legacy nonce contract while the docstring claims RunContext compatibility. runtime-semantics found no defects; performance confirmed the hot path is unchanged. Cross-repo agent verified all builder-kit contract surfaces are consistent against the actual ENGN-8646 branch.


Generated by code-review-forge — multi-agent PR review with temporal redundancy

@TaniBuilds TaniBuilds left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🔍 Multi-Agent Inline Review

12 inline findings posted as resolvable threads.

3 findings not on changed lines (see PR comment for full report): 🟡 1 medium, 🔵 2 low

Generated by code-review-forge

Comment thread allora_forge_builder_kit/worker_runtime.py Outdated
Comment thread pyproject.toml
Comment thread allora_forge_builder_kit/worker_runtime.py Outdated
Comment thread allora_forge_builder_kit/worker_runtime.py Outdated
Comment thread allora_forge_builder_kit/worker_runtime.py Outdated
Comment thread allora_forge_builder_kit/worker_runtime.py Outdated
Comment thread pyproject.toml
Comment thread allora_forge_builder_kit/worker_runtime.py
Comment thread allora_forge_builder_kit/worker_runtime.py Outdated
Comment thread allora_forge_builder_kit/worker_runtime.py Outdated
…t loop

Managed custody calls AlloraWalletConfig.from_env(), which performs a blocking
wallet-info HTTPS fetch. Doing it inside the async _run() stalled the event loop
at startup (and delayed cancellation if the backend hung). Move resolution into a
sync _resolve_wallet_cfg() helper invoked from main() before asyncio.run(), and
pass the pre-resolved wallet_cfg into _run(). Addresses synth-003.
…ssion drops

run_fn unconditionally called raw_fn(ctx.nonce). An artifact written to the newer
RunContext contract (raw_fn(ctx)) would get an int, raise AttributeError inside
Inferer.submit, and be silently caught — dropping every submission with no alarm.
Resolve the artifact's call shape once at load via inspect.signature (defaulting to
the legacy nonce form for back-compat) and document the supported artifact contract
in _run's docstring. Addresses synth-005.
The inferer callback closure had no parameter or return annotation. Import
RunContext under a TYPE_CHECKING guard and annotate run_fn(ctx: RunContext) -> float
so pyright catches call-site type errors (e.g. a future ctx.nonce rename).
Addresses synth-009.
The custody routing parameter only ever takes two values. Narrow it from a bare
str to Literal["local", "managed"] (the resolution moved into _resolve_wallet_cfg)
per the strong-typing mandate, so direct callers and pyright catch invalid values
instead of silently falling through. argparse choices still enforce the CLI
boundary. Addresses synth-008.
Passing --custody managed --mnemonic-file silently discarded the key file (managed
custody resolves the wallet from FORGE_API_KEY), leaving an operator to believe a
local key was in play. Surface the conflict as a parser.error before any network
calls. Addresses synth-004.
Fail fast with a clear usage error when --custody managed is used without
FORGE_API_KEY, naming the missing variable instead of surfacing it deep in the SDK.
Also align the _resolve_wallet_cfg docstring with the real auto-provision SDK
contract: FORGE_API_KEY (no FORGE_SIGNING_WALLET_ID) yields a deferred managed
config and takes precedence over PRIVATE_KEY/MNEMONIC, so there is no silent
local-key fallback. Hardening for synth-001 (false positive against the wrong SDK
branch; see PR reply).
A managed (Privy) wallet never receives faucet drips, so gasless submission
requires a fee granter; without FEE_GRANTER every transaction fails with
'insufficient fees'. Emit a startup warning naming the variable. Note: unlike the
reviewer's claim, FORGE_SIGNING_WALLET_ID is NOT required — the auto-provision SDK
defers wallet creation when only FORGE_API_KEY is set. Addresses synth-006.
Add a module docstring clarifying that ALLORA_API_KEY (faucet/consumer, via
--api-key) and FORGE_API_KEY (managed-custody backend) are different secrets for
different backends, and that --custody managed reads FORGE_API_KEY only. Reduces
the naming-collision footgun. Addresses synth-012.
…patch

Add pytest cases asserting custody='managed' routes to AlloraWalletConfig.from_env()
without reading mnemonic_file, custody='local' builds AlloraWalletConfig(mnemonic_file=...)
without calling from_env(), the managed+mnemonic-file and managed-without-FORGE_API_KEY
CLI guards exit, and _artifact_expects_context distinguishes legacy nonce vs RunContext
artifacts. Addresses synth-007.
…erer

The raw example still used the removed AlloraWorker(...) direct constructor with a
predict_fn(nonce) signature, which would break against the branch SDK this PR
targets. Mirror worker_runtime.py: switch to AlloraWorker.inferer(...) and wrap the
pickled nonce fn in a RunContext adapter (_run_fn). Addresses synth-010.
@TaniBuilds

Copy link
Copy Markdown
Contributor Author

Automated-review triage — actioned

Worked through all 12 unresolved inline threads plus the 3 off-line findings (synth-007/010/012). All 12 threads are now resolved with replies. One commit per issue; the local-dev [tool.uv.sources] pin was intentionally kept.

A) Fixed (commit → finding)

Commit Finding Change
07c525c synth-003 (MED) Resolve wallet config in a sync _resolve_wallet_cfg() from main() before asyncio.run(); pass pre-resolved wallet_cfg into _run() so the blocking from_env() fetch can't stall the event loop.
e5fd3e4 synth-005 (MED) Detect the artifact call-shape once via inspect.signature (_artifact_expects_context), dispatch raw_fn(ctx) vs raw_fn(ctx.nonce), default to legacy nonce form; document the artifact contract. Prevents silent per-submission drops.
d391b1d synth-009 (style) Annotate run_fn(ctx: RunContext) -> float with a TYPE_CHECKING import.
da015ff synth-008 (style) Type custody as Literal["local", "managed"] (on _resolve_wallet_cfg).
ff77d4a synth-004 (MED) parser.error when --custody managed is combined with --mnemonic-file.
f075817 synth-001 (hardening) Fail fast in main() when --custody managed and FORGE_API_KEY is unset; align docstring with the real SDK contract.
b933c1d synth-006 (MED) Warn at startup when FEE_GRANTER is unset under managed custody.
b5e25d8 synth-012 (LOW) Module docstring distinguishing ALLORA_API_KEY (faucet/consumer) from FORGE_API_KEY (managed backend).
2e734b5 synth-007 (MED) Tests for custody routing, the two CLI guards, and artifact call-shape detection (11 new cases).
cd58993 synth-010 (LOW) Migrate notebooks/deploy_worker_raw.py to AlloraWorker.inferer(...) + a RunContext adapter.

B) Declined / no code change (with reason)

  • synth-001 (CRITICAL) — FALSE POSITIVE. The reviewer inspected the wrong sibling SDK branch (tani/ENGN-8615-sdk-privy-delegated-signing). The pin actually resolves to tani/engn-8646-auto-provision-managed-wallets (allora-sdk-py PR #81), where the documented happy path provably exists: deferred-managed from_env() (config.py:58-66), __post_init__ allows zero local sources when managed (config.py:83-84), managed branch is evaluated before PRIVATE_KEY/MNEMONIC (so managed takes precedence — no silent local fallback), provision_wallet/clear_association (remote_signer.py:138/:150), and test_from_env_defers_managed_provision. The PR does not crash and does not silently build a local wallet. Added the FORGE_API_KEY guard (f075817) as genuine hardening anyway.
  • synth-002 (HIGH) — intentional. The [tool.uv.sources] pin is deliberate local-dev for this draft and is flagged for removal + version bump at SDK release (PR description "Not for merge as-is"). Not deleting it here. The grep && exit 1 CI guard is deferred to the protected base branch (it would fail this draft's own CI, where the pin is intentionally present).
  • synth-006 — partially declined. FORGE_SIGNING_WALLET_ID is not required (same wrong-branch premise as synth-001; the SDK defers provisioning when only FORGE_API_KEY is set), so no such check was added. The needed parts (FORGE_API_KEY guard, FEE_GRANTER warning) were done.
  • synth-011 (LOW) — clarified the ENGN-8646 SDK branch exists (allora-sdk-py PR #81); no code change.
  • synth-013 (LOW).inferer(...) enabling the sanity check by default is confirmed and kept (useful guard, failures don't block submission, RPC throttled to 60s); documented rather than disabled.
  • synth-014 (LOW) — env-driven config is acceptable for the draft; --forge-* CLI flags tracked as an optional follow-up.
  • synth-015 (INFO) — cross-repo; no kit change. Provisioning is memoized at startup in the SDK (init_worker_wallet), not per-nonce.

C) Needs user attention

  • No blocking items / no unresolved threads. All 12 resolved.
  • The CRITICAL was a false positive — please note the automated reviewer matched the wrong SDK branch; the auto-provision path is real and lives in allora-sdk-py PR #81.
  • Before un-drafting: remove the [tool.uv.sources] pin + bump allora-sdk (already in the PR plan), and consider adding the pin CI guard on the base branch then.
  • Heads-up (pre-existing, unrelated): tests/test_wallet_link.py::test_sign_roundtrip_with_cosmpy fails on a cosmpy API drift (PrivateKey.public_key_bytes) from the ENGN-8614 base commit — untouched by this PR.

D) Verification

  • python3 -m py_compile — clean on all changed files.
  • SDK pin resolves: allora_sdk imports from the ENGN-8646 auto-provision worktree (AlloraWorker, RunContext, AlloraWalletConfig).
  • uv run python -c "import allora_forge_builder_kit.worker_runtime" — OK.
  • uv run pytest tests/test_worker_runtime.py -q14 passed.
  • uv run pytest tests/ -q76 passed, 23 skipped, 1 failed (the 1 failure is the pre-existing, unrelated test_wallet_link cosmpy drift above). Note: the venv needed uv sync --extra dev to provision pytest + core deps.
  • ruff / pyright: not configured in this repo — skipped.

@TaniBuilds TaniBuilds left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🔍 Multi-Agent Inline Review

16 inline findings posted as resolvable threads.

Generated by code-review-forge

@@ -77,16 +150,43 @@ def main() -> None:
parser.add_argument("--mnemonic-file", default=None, help="Path to wallet key file (managed by WorkerManager)")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🟡 MEDIUM · ✅ correctness · synth-001

--custody local default + no --mnemonic-file → SDK getpass() hangs headless workers or silently creates an unfunded wallet

When --custody local (the default) is used without --mnemonic-file, _resolve_wallet_cfg('local', None) returns None. The ENGN-8646 SDK's init_worker_wallet(None, topic_id) falls through to: (a) read .allora_key from CWD if present, OR (b) call getpass('Mnemonic: ') for interactive input, AND if the operator presses Enter, silently generates a fresh mnemonic and writes it to .allora_key in CWD.

Consequences:

  1. In headless/supervised mode (WorkerManager, systemd, Docker without TTY), the worker hangs indefinitely on getpass. Operators see a worker process that never starts producing submissions.
  2. If getpass succeeds with empty input, the SDK creates a brand-new, unfunded wallet the operator did not consciously create. Combined with --no-faucet, the worker signs with a zero-balance wallet and every tx fails.
  3. The .allora_key CWD path is easy to confuse with the kit's _load_api_key fallback paths.

The three --custody managed guards in main() use parser.error() for strict pre-flight validation; the --custody local path has no symmetric guard. No Phase-A agent flagged this — the adversarial reviewer caught it by inspecting the ENGN-8646 SDK branch directly (utils.py lines 47-64).

Suggested change
parser.add_argument("--mnemonic-file", default=None, help="Path to wallet key file (managed by WorkerManager)")
if args.custody == "local" and not args.mnemonic_file and not os.path.exists(".allora_key"):
parser.error(
"--custody local requires --mnemonic-file or an existing .allora_key in CWD; "
"otherwise the SDK falls back to an interactive getpass() prompt and hangs in headless mode"
)

🔗 Cross-repo: affects allora-sdk-py

Comment thread pyproject.toml
@@ -42,3 +42,12 @@ Homepage = "https://github.com/allora-network/allora-forge-builder-kit"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🟡 MEDIUM · 🏗️ architecture · synth-002

allora-sdk>=1.0.6 dependency floor does not gate on AlloraWorker.inferer — clean pip install . fails at runtime

The PR switches from AlloraWorker(run=run_fn, ...) to AlloraWorker.inferer(run=run_fn, ...) (worker_runtime.py line 133, notebooks/deploy_worker_raw.py line 64). The .inferer() classmethod factory exists on the sibling tani/engn-8646-auto-provision-managed-wallets branch but is absent from the published allora-sdk==1.0.6 release on PyPI.

The [tool.uv.sources] block hides this for the author's machine (uv resolves the local worktree), but a clean pip install . — which ignores [tool.uv.sources] — resolves allora-sdk==1.0.6 from PyPI and worker_runtime.py fails at the first worker construction with AttributeError: type object 'AlloraWorker' has no attribute 'inferer'. Any downstream user, GitHub Actions step using pip, or wheel build is affected. The declared dependency floor is therefore wrong for the actual API surface the kit now uses.

💡 Suggestion: Bump the allora-sdk dependency floor in [project].dependencies to the first release that ships AlloraWorker.inferer and the deferred-managed from_env() path. Until that release exists, mark the kit version as a pre-release and document in the README that uv + the sibling worktree pin is mandatory for installation. Add a CI matrix step that installs via plain pip install . (no uv, no local path) to catch floor regressions.

🔗 Cross-repo: affects allora-sdk-py

return cfg


def _resolve_wallet_cfg(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🟡 MEDIUM · 🔒 security · synth-003

Kit lacks defensive guard rejecting PRIVATE_KEY/MNEMONIC/MNEMONIC_FILE under managed custody — wrong-SDK-branch resolution causes silent credential confusion

🤝 flagged by 2 agents: security, adversarial

_resolve_wallet_cfg documents "FORGE_API_KEY always takes precedence — there is no silent local-key fallback." This claim is correct ONLY on the tani/engn-8646-auto-provision-managed-wallets branch (confirmed by cross-repo agent via direct inspection). However, the [tool.uv.sources] pin is a CWD-relative path with no branch or SHA constraint. If a reviewer, CI machine, or downstream consumer has the wrong SDK branch checked out (tani/ENGN-8615-sdk-privy-delegated-signing or any released SDK without ENGN-8646), AlloraWalletConfig.from_env() falls through to read PRIVATE_KEY/MNEMONIC/MNEMONIC_FILE. An operator running --custody managed with stale local-key env vars would silently sign transactions with their local key while believing they are using Privy-managed custody.

The kit's only managed-custody guard is FORGE_API_KEY presence (line 170-174); it does NOT detect or reject conflicting local-key env vars. The --mnemonic-file mutual-exclusion guard catches the explicit flag but not the env vars that from_env() reads.

🎭 adversarial note: Phase B (adv-5) challenged sec-001's HIGH severity: cross-repo agent verified the code IS correct against the actual pinned ENGN-8646 branch — 'FORGE_API_KEY always takes precedence' is accurate there. The threat model driving HIGH severity is 'wrong SDK branch resolved by the path pin', which is a build-config gap, not a code bug. Treated as defense-in-depth at MEDIUM. The primary remediation should be replacing the relative path pin with a git+SHA reference (synth-002/synth-015); the kit-side env-var rejection guard is belt-and-suspenders.

Suggested change
def _resolve_wallet_cfg(
def _resolve_wallet_cfg(
custody: Literal["local", "managed"], mnemonic_file: str | None
) -> AlloraWalletConfig | None:
if custody == "managed":
# Fail closed: under any SDK without the ENGN-8646 deferred-managed path,
# from_env() would silently fall through to PRIVATE_KEY/MNEMONIC/MNEMONIC_FILE.
# Reject those env vars explicitly so managed custody can never sign with a local key.
stale = [v for v in ("PRIVATE_KEY", "MNEMONIC", "MNEMONIC_FILE") if os.environ.get(v)]
if stale:
raise RuntimeError(
"--custody managed refuses to proceed with local-key env vars set "
f"({', '.join(stale)}); clear them or use --custody local"
)
return AlloraWalletConfig.from_env()
return AlloraWalletConfig(mnemonic_file=mnemonic_file) if mnemonic_file else None

🔗 Cross-repo: affects allora-sdk-py

from allora_sdk.worker.context import RunContext


def _load_api_key(explicit: str | None) -> str:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🟡 MEDIUM · 🔒 security · synth-004

_load_api_key falls back to CWD-relative key files with no ownership check

_load_api_key (lines 31-41) falls back to os.path.exists / open on "notebooks/.allora_api_key" and ".allora_api_key" — both CWD-relative with no canonicalization or file-ownership check. A worker started in an attacker-writable scratch directory silently reads an attacker-supplied consumer key, causing the worker to authenticate to the Allora consumer/faucet API under an attacker-chosen identity (polluting attribution, draining the attacker's faucet quota, or confusing inference records). Under --custody managed, where the operator may have deliberately avoided local key material, this CWD fallback still loads an implicit key from disk without any indication.

This is a pre-existing pattern not introduced by this PR, but is now load-bearing for the new managed-custody flow since ALLORA_API_KEY is still required even under --custody managed.

💡 Suggestion: Resolve the key-file path against the kit's install location (os.path.dirname(__file__)) rather than CWD, and verify the file is not world-writable (os.stat mode check) before reading. Or remove the file fallback entirely and require ALLORA_API_KEY / --api-key explicitly.

return AlloraWalletConfig(mnemonic_file=mnemonic_file) if mnemonic_file else None


def _artifact_expects_context(raw_fn: Callable[..., object]) -> bool:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🔵 LOW · ✅ correctness · synth-005

_artifact_expects_context has three distinct misclassification modes: name heuristic, substring annotation match, and class artifacts

🤝 flagged by 3 agents: correctness, deep-analysis, adversarial

Three failure modes in _artifact_expects_context (lines 76-95):

1. Name-based dispatch false-positives (correctness-2, DA-001): Parameters named ctx, context, or run_context trigger RunContext dispatch regardless of type. A legacy artifact def predict(ctx): return float(ctx) * scale — where ctx is the author's name for the integer nonce — will receive a RunContext object and raise TypeError / AttributeError on every inference cycle. This appears as silent topic non-participation because the SDK's Inferer catches and logs the error per submission without raising. The universe of pre-existing artifacts using these param names is small but non-zero (cloudpickle preserves source names verbatim).

2. Substring annotation match (adv-3): "RunContext" in annotation matches MyRunContext, FastRunContextWrapper, WorkerRunContextV2, and any user-defined class containing "RunContext" as a substring. Such artifacts would receive an SDK RunContext instance instead of their custom type. Fix: exact-token match on the trailing qualified-name component.

3. Class-based artifacts (adv-6): inspect.signature on a class returns the __init__ signature, not __call__. A pickled class whose __init__ takes a param named ctx would trigger false-positive RunContext dispatch — constructing a new instance per nonce instead of calling the intended prediction entry point. Tests do not cover the class case.

🎭 adversarial note: Phase B (adv-1) challenged DA-001's HIGH severity as inflated (matching correctness-2's LOW) and warned that DA-001's proposed execution-based probe would replace a safe static check with a side-effect-prone runtime probe of untrusted cloudpickle-deserialized code — double-executing non-idempotent artifacts and introducing broad TypeError/AttributeError catch-all logic. The current static inspect.signature approach is architecturally correct; only the matching logic needs tightening. Severity kept at LOW.

Suggested change
def _artifact_expects_context(raw_fn: Callable[..., object]) -> bool:
annotation = "" if params[0].annotation is inspect.Parameter.empty else str(params[0].annotation)
# Match the exact short name (handles forward-ref strings, qualified names, and quoted strings)
# to avoid matching MyRunContext, FastRunContextWrapper, etc.
short = annotation.strip("'\"").rsplit(".", 1)[-1]
return short == "RunContext" or params[0].name in ("ctx", "context", "run_context")

@@ -77,16 +150,43 @@ def main() -> None:
parser.add_argument("--mnemonic-file", default=None, help="Path to wallet key file (managed by WorkerManager)")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

INFO · ✅ correctness · synth-012

--api-key argument missing help text — invites confusion between ALLORA_API_KEY and FORGE_API_KEY under --custody managed

parser.add_argument("--api-key", default=None) has no help= string (line 149). The module docstring carefully distinguishes ALLORA_API_KEY (consumer/faucet) from FORGE_API_KEY (Forge backend, managed wallet), but --help shows --api-key API_KEY with no description. An operator running --custody managed who sees an "ALLORA_API_KEY not found" error from _load_api_key may incorrectly supply their FORGE_API_KEY via --api-key, causing a 401 from the topic-query API or leaking the Forge key in shell history.

Suggested change
parser.add_argument("--mnemonic-file", default=None, help="Path to wallet key file (managed by WorkerManager)")
parser.add_argument("--api-key", default=None, help="Allora consumer API key (ALLORA_API_KEY) for faucet/topic queries — distinct from FORGE_API_KEY used by --custody managed")

@@ -77,16 +150,43 @@ def main() -> None:
parser.add_argument("--mnemonic-file", default=None, help="Path to wallet key file (managed by WorkerManager)")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

INFO · 🎨 style · synth-013

--mnemonic-file help text is stale with respect to the new --custody option

--mnemonic-file help says "Path to wallet key file (managed by WorkerManager)" (line 150). With --custody local now exposed as the default, this flag is the user-facing way to point a self-custodial worker at a key file — not exclusively a WorkerManager-internal concern. The word "managed" also conflicts with --custody managed (Privy-managed wallets vs WorkerManager-managed), and the new error message says --mnemonic-file is incompatible with --custody managed, which confuses the two senses of "managed".

Suggested change
parser.add_argument("--mnemonic-file", default=None, help="Path to wallet key file (managed by WorkerManager)")
parser.add_argument("--mnemonic-file", default=None, help="Path to wallet mnemonic file (used with --custody local; mutually exclusive with --custody managed)")

return AlloraWalletConfig(mnemonic_file=mnemonic_file) if mnemonic_file else None


def _artifact_expects_context(raw_fn: Callable[..., object]) -> bool:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

INFO · 🎨 style · synth-014

_artifact_expects_context docstring does not enumerate the accepted context-indicator parameter names

The docstring (lines 77-83) says the function switches when the parameter is "named like a context" but never lists the accepted names. The actual implementation accepts exactly ("ctx", "context", "run_context") (line 95). context in particular is extremely common in Python (SSL context, logging context, Flask/Django request context, etc.). Without listing these names, developers building new artifacts cannot predict which parameter names will trigger RunContext dispatch — the misclassification risk documented in synth-005 is invisible from the public docstring.

💡 Suggestion: Update the docstring to say: "named exactly as one of: ctx, context, or run_context" and note that any other name (e.g. nonce, value, i) keeps the legacy fn(nonce: int) dispatch.

Comment thread pyproject.toml
# uv picks this up on `uv sync` / `uv run`. REMOVE this block and bump the
# `allora-sdk` version in [project].dependencies once the SDK release with ENGN-8646
# ships; the relative path assumes the allora-sdk-py worktree is a sibling directory.
[tool.uv.sources]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

INFO · 🔗 cross-repo · synth-015

Cross-repo sibling manifest captured the wrong allora-sdk-py branch — causes false-positive reviews

🤝 flagged by 2 agents: cross-repo, architecture

The sibling manifest matched allora-sdk-py@tani/ENGN-8615-sdk-privy-delegated-signing, and the captured diff is that branch's diff. The PR's [tool.uv.sources] pin resolves allora-sdk to ../allora-sdk-py, whose worktree carries tani/engn-8646-auto-provision-managed-wallets (allora-sdk-py PR #81) — a superset with the deferred-managed from_env() path, provision_wallet, clear_association, and AlloraWorker.inferer.

The cross-repo agent verified directly against the ENGN-8646 branch that all builder-kit contract surfaces are consistent (from_env() deferred-managed path, RunContext.nonce, init_worker_wallet with topic_id, etc.). The prior pass's synth-001 CRITICAL and synth-011 LOW findings were false positives caused by this manifest mismatch.

Until the manifest is updated, every automated review of this PR will produce the same false positives from agents that inspect the wrong sibling branch.

💡 Suggestion: Update the cross-repo sibling manifest to include allora-sdk-py@tani/engn-8646-auto-provision-managed-wallets as the primary sibling (or in addition to ENGN-8615). Add a comment in the [tool.uv.sources] block naming the exact branch: # branch: tani/engn-8646-auto-provision-managed-wallets (allora-sdk-py PR #81).

🔗 Cross-repo: affects allora-sdk-py

return AlloraWalletConfig(mnemonic_file=mnemonic_file) if mnemonic_file else None


def _artifact_expects_context(raw_fn: Callable[..., object]) -> bool:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

INFO · 🏗️ architecture · synth-016

Signature dispatch reverse-engineers SDK call conventions by string-matching instead of consuming a stable SDK adapter

_artifact_expects_context (lines 76-95) decides how to invoke the pickled artifact by string-matching parameter names and annotation strings — reverse-engineering the SDK's TRunFn = Callable[[RunContext], ...] contract. This is fragile against: (a) the SDK renaming RunContext; (b) alternate context types the SDK may add (e.g., ReputerContext already exists in reputer.py:31); (c) artifacts whose param is named ctx but expects a different type.

Architecturally, the legacy-nonce→RunContext adapter belongs in the SDK (which owns the RunContext type and the TRunFn contract) as a documented wrap_legacy_nonce_fn / from_legacy_nonce_fn helper, not in a downstream kit doing reflective introspection on cloudpickled callables. The SDK already has make_reputer_function in worker/utils.py as precedent. The kit has no compile-time guarantee that its dispatch matches what inferer.submit actually passes.

💡 Suggestion: Open a follow-up on allora-sdk-py to expose a allora_sdk.worker.utils.legacy_nonce_fn(fn: Callable[[int], float]) -> Callable[[RunContext], float] adapter. The kit would call it for legacy artifacts and pass RunContext-typed artifacts through unchanged, delegating contract ownership to the SDK.

🔗 Cross-repo: affects allora-sdk-py

Base automatically changed from tani/ENGN-8614-worker-wallet-claim to main July 21, 2026 18:44
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.

1 participant