feat: managed-custody worker lifecycle in WorkerManager (ENGN-8646) - #35
feat: managed-custody worker lifecycle in WorkerManager (ENGN-8646)#35TaniBuilds wants to merge 15 commits into
Conversation
WorkerManager can now run Privy-managed workers end-to-end, not just local self-custody: - deploy_worker(custody="managed") provisions (idempotent get-or-create) a managed wallet bound to the topic via the Forge backend and registers the worker against the backend-assigned address — no local mnemonic/key file. One wallet per topic; a re-deploy refreshes the artifact on the same wallet. - start_worker launches managed workers with --custody managed and injects FORGE_API_KEY/FORGE_BACKEND_URL into the subprocess env (no --mnemonic-file). - remove_worker releases the (user, topic) binding via clear-association on decommission (best-effort: local removal always proceeds). - workers table gains custody + signing_wallet_id (lightweight migration); Forge client built lazily from FORGE_* env (injectable for tests). Reuses allora_sdk's ForgeBackendClient (provision_wallet + new clear_association). Tests cover provision/register, idempotent redeploy, managed command/env construction, clear-on-remove (incl. failure tolerance), local-not-cleared, and the missing-credentials error.
🔍 Multi-Agent Multi-Repo Code Review 🧑💻20 findings across 9 dimensions — 🟠 1 high · 🟡 8 medium · 🔵 11 low 🧵 Synthesized by Dimensions: 🚨 Must Fix
Consequence: managed custody is a domain-layer feature with no presentation/wiring layer update. An operator cannot deploy, start, or reconcile managed workers using any shipped tool — only by writing Python directly against WorkerManager. 💡 Add forge_api_key/forge_backend_url to _make_manager's WorkerManager construction (env-derived via os.environ fallback, no new CLI flags required). Add a 'deploy' subcommand accepting --topic, --artifact, --custody {local,managed} so managed deploys are reachable from the shipped CLI. Note: arch-003 (eternal reconcile loop) is a symptom of the same gap and should be fixed concurrently. ✅ Correctness (5 findings)🟡 synth-p1-002 (medium) — 🤝 flagged by 2 agents: correctness, adversarial
The lazy loader in Additionally, the 💡 Either (a) gate this PR on a published allora-sdk release containing provision_wallet/clear_association and bump the lower bound accordingly, dropping the [tool.uv.sources] LOCAL-DEV PIN block from the publishable pyproject.toml; or (b) add a runtime 🔗 Cross-repo: affects allora-sdk-py 🔵 synth-p1-010 (low) —
This is pre-existing behavior that the refactor (extracting _build_run_command) preserved verbatim — the new try/except around _build_run_command shows the contributor was thinking about cleanup but missed widening it to cover Popen. 💡 Widen the try block (or use try/finally) to also close log_f when Popen raises. On success ownership transfers to the runner dict; on failure, log_f.close() must run. 🔵 synth-p1-014 (low) —
💡 Before INSERTing a fresh managed row, look up any existing managed worker for topic_id (any address). If one exists with a different address than info.address, either (a) refuse with a clear error pointing at remove_worker(topic_id, old_address), or (b) atomically clean up the orphaned row and call clear_association(old_signing_wallet_id) first. 🔵 synth-p1-015 (low) —
💡 After the emptiness check, also assert that info.address starts with the expected bech32 prefix for self._network (e.g. 'allo1'). Raise RuntimeError with a clear message naming the expected vs returned prefix. This catches a misconfigured backend before any DB write. 🔵 synth-p1-019 (low) —
Failure mode: managed worker subprocess starts, RemoteWallet builds successfully, the first submission fails with an insufficient-fee error from the chain. reconcile() re-attempts forever (no terminal misconfigured state per synth-p1-006). 💡 Either (a) accept fee_granter on WorkerManager.init (defaulting to $FEE_GRANTER) and explicitly inject it into the managed env; or (b) emit logger.warning at deploy_worker(custody='managed') time when FEE_GRANTER is absent from the environment, naming the SDK recommendation. Add an equivalent precheck in build_run_command for parity with the existing FORGE* credential checks. 🔗 Cross-repo: affects allora-sdk-py 🔒 Security (4 findings)🟡 synth-p1-003 (medium) —
Step 3 can fail for many reasons: ENOSPC on shutil.copy2, sqlite3.OperationalError (database locked, disk full), sqlite3.IntegrityError from a concurrent deploy race, or SIGTERM/KeyboardInterrupt. When this happens, the backend binding survives (provision_wallet is idempotent at (user, topic) level, so a retry recovers the same wallet) BUT the operator only recovers the binding by re-running deploy_worker. If they instead run remove_worker to clean up, _get_custody returns ('local', None) (no row exists) and no clear_association is ever issued. The backend binding leaks permanently — the provisioned wallet_id was never persisted locally. 💡 Wrap the provision_wallet → add_worker sequence in a try/except: on any exception after a successful provision_wallet, log the provisioned wallet_id+address at WARN level (so an operator can grep logs) and best-effort call clear_association(info.id) to release the backend binding. Alternatively, stage a tombstone row (signing_wallet_id=NULL, custody='managed_provisioning') before the provision_wallet call, update it on success, and let reconcile() detect and clean up stale tombstones on restart. 🔗 Cross-repo: affects allora-sdk-py, forge-v2 🟡 synth-p1-004 (medium) —
If the process is killed (SIGKILL, OOM, host reboot) between step 3 and step 4, the Forge backend retains an active (user, topic) → wallet_id binding that authorizes delegated Privy signing, while the local registry shows no worker and no pending-release record. There is no way to enumerate orphaned bindings from the kit; the backend binding leaks permanently. This is distinct from the prior synth-004 concern (exception swallowing): even if clear_association succeeds, the window between DELETE and clear_association is non-retryable because signing_wallet_id is never persisted to a durable cleanup queue — it only exists as a local variable at that point. Note: The author left synth-004 open as a 'product decision'; this finding adds the stronger ordering argument as a new angle. 💡 Reverse the ordering for managed custody: call clear_association FIRST (while the row still exists), then DELETE only after the backend acknowledges release. If clear_association fails, either leave the row in a 'pending_release' status (so reconcile can retry) or log the signing_wallet_id to a durable cleanup log. At minimum this reduces the SIGKILL window from 'before every failure' to only the clear_association network call duration. 🔗 Cross-repo: affects allora-sdk-py, forge-v2 🟡 synth-p1-005 (medium) —
If the backend ever returns the same address but a different wallet_id for the same topic (wallet cleared/regenerated server-side, backend migration, DR event), While this backend invariant likely holds today, a future backend migration or incident could violate it silently, and the kit has no observability hook to detect the rotation. 💡 Before overwriting via _update_worker, read the current signing_wallet_id from the DB. If it is non-NULL and != info.id, log a WARN ('backend rotated wallet for topic {topic_id}: old={old_id} new={info.id}') and best-effort call clear_association(old_id) so the prior binding does not leak. Alternatively, refuse the redeploy and surface a clear error directing the operator to remove_worker + redeploy. 🔗 Cross-repo: affects allora-sdk-py, forge-v2 🔵 synth-p1-018 (low) — 🤝 flagged by 2 agents: adversarial, performance
Additionally, copying os.environ N times during start_all/reconcile for N managed workers is avoidable per-startup allocation (though low severity given typical N). 💡 Build a minimal env dict instead of os.environ.copy(): start from an allowlist of std vars + ALLORA_API_KEY + FEE_GRANTER, then add the FORGE_* trio. This makes the credential surface auditable and reduces accidental secret propagation. 🏗️ Architecture (4 findings)🟡 synth-p1-006 (medium) —
The lifecycle state machine has no terminal 'misconfigured' state for permanent config errors: 'stopped' means 'should be started', so any permanent configuration defect becomes an eternal log-spam loop with no operator-actionable signal. A managed worker can reach this state via: (a) DB row exists but FORGE_API_KEY was rotated/unset; (b) signing_wallet_id is NULL; (c) workerctl reconcile run without forge credentials (the workerctl gap in synth-p1-001). 💡 Extend the status or add an error-count field so reconcile() can transition a permanently-failing row to a terminal 'misconfigured' state (or set enabled=0 + write last_error) after N consecutive failures for the same row. At minimum, distinguish permanent-config ValueError from transient subprocess failures and skip rows in the terminal state rather than re-attempting every cycle. Surface the terminal-state rows in the returned 🔵 synth-p1-011 (low) — 🤝 flagged by 2 agents: architecture, adversarial
Adversarial note: the UNIQUE constraint is an adequate correctness floor — no data is corrupted, only a UX wart. Severity is LOW. Any lock-wrap fix should also cover the local explicit-address branch (lines 412-454) which has the same pattern and is the original source of the missing-lock discipline. 🎭 adversarial note: adv-pr35-6 challenges arch-002's medium severity: the UNIQUE(topic_id, address) constraint is the safety net — worst case is a confusing-but-clear ValueError to the losing thread, not data corruption. Severity demoted from medium to low. Any fix should also extend to the local explicit-address branch (lines 412-454) which has the same missing-lock pattern. 💡 Wrap the provision_wallet → _worker_exists → add_worker/_update_worker sequence in 🔵 synth-p1-012 (low) — 🤝 flagged by 2 agents: architecture, adversarial
🎭 adversarial note: adv-pr35-7 partially challenges arch-004: the SDK deferred-provisioning capability the comment describes IS real (from_env + init_worker_wallet). The suggested rewrite should preserve that info rather than dropping it entirely. The revised suggested_code above incorporates this nuance. 💡 Rewrite to document both cases: (a) the kit's path: WorkerManager pre-provisions the topic-bound wallet and pins FORGE_SIGNING_WALLET_ID; (b) the SDK's deferred path (available when only FORGE_API_KEY is set, used by callers that bypass WorkerManager). This preserves documentation of the SDK's broader contract while clarifying which path the kit takes. 🔵 synth-p1-013 (low) —
💡 In init, when forge_client is not None, verify it at the boundary: ⚡ Performance (1 findings)🔵 synth-p1-017 (low) — 🤝 flagged by 2 agents: performance, security
Additionally, deploy_worker does not hold self._lock during this sequence, so two concurrent deploys for the same topic both call provision_wallet in parallel, doubling the API-key-bearing requests. The provision_wallet call is the only place the Forge API key leaves the process over the network, so minimizing its frequency reduces attack surface. 💡 Read the existing worker row for (topic_id) first; if a managed row with a non-null signing_wallet_id exists and replace=False, reuse the stored address/wallet_id and skip the provision_wallet call, calling the backend only when the row is missing or signing_wallet_id is null. If re-provisioning is required for correctness on every deploy, document it as an explicit contract and add a force_reprovision flag. 🔗 Cross-repo: affects allora-sdk-py, forge-v2 🎨 Style (4 findings)🟡 synth-p1-007 (medium) —
💡 Re-raise as ImportError instead of ValueError: 🟡 synth-p1-008 (medium) —
The Allora Python style guide explicitly prefers structured types (dataclasses / TypedDict) over raw dicts with magic indices. A future column addition to either SELECT that misaligns the indices would be a silent wrong-value bug (no test failure, no type error). Using 💡 Set 🔵 synth-p1-016 (low) —
💡 Change 🔵 synth-p1-020 (low) —
💡 Add 🔗 Cross Repo (1 findings)🟡 synth-p1-009 (medium) —
Consequence: an operator who sets only 💡 Drop the explicit FORGE_BACKEND_URL requirement from WorkerManager and pass 🔗 Cross-repo: affects allora-sdk-py, allora-forge-builder-kit 📝 Agent Summaries🎭 adversarial: Phase B (adversarial red-team): Phase A covered the major correctness/security gaps well, but missed three durability concerns symmetric across the managed-custody lifecycle: (1) provision-then-write ordering on deploy leaks backend bindings if the write fails (medium, symmetric to sec-001 on remove); (2) silent overwrite of signing_wallet_id by _update_worker leaks the prior backend binding if the backend ever rotates the per-topic wallet (medium); (3) missing bech32-prefix validation on the provisioned address before INSERT (low). Two operational/security gaps: (4) os.environ.copy() leaks the full parent environment into the managed subprocess (low, least-privilege), and (5) the SDK-recommended FEE_GRANTER pairing for managed signing is never plumbed by WorkerManager, leaving operators to discover this via cryptic chain errors (low). Challenges to Phase A: arch-002 is real but mitigated by UNIQUE(topic_id, address), suggesting a severity demotion from medium to low, and the suggested fix should be extended to the local explicit-address branch for consistency. arch-004 is partially valid — the worker_runtime.py comment is misleading IN CONTEXT but the SDK's deferred-provisioning capability is real, so the rewrite should preserve that. correctness-pr35-1 is confirmed with an additional observation that pyproject.toml's [tool.uv.sources] block ships in built wheels (pip ignores it), making the pip-user UX gap a real blocker. 🏗️ architecture: The managed-custody addition is well-isolated behind a ForgeClientProtocol and the custody-discriminant column design is sound, but the feature is wired only into the WorkerManager core: neither the workerctl CLI nor the web_dashboard (the only shipped operator entry points) construct a WorkerManager with forge credentials or expose a managed deploy command, so the path is unreachable in any shipped configuration. Two lifecycle gaps remain: _deploy_managed_worker mutates state without the manager RLock (a TOCTOU race against the UNIQUE(topic_id,address) constraint), and reconcile() catches the new ValueError from _build_run_command but leaves enabled=1, so a misconfigured managed worker spins in an eternal log-spam loop. The worker_runtime.py comment also mis-describes the contract by claiming the subprocess provisions the wallet when in fact WorkerManager provisions it pre-spawn. ✅ correctness: Most previously-flagged correctness issues have been addressed in subsequent commits (status_all parity, managed local-only-input rejection, managed redeploy reject_zero/wallet sync, replaced-vs-reused action, ForgeClientProtocol, identity_ref sentinel, lock-guarded lazy SDK build, friendly ImportError fallback, malformed-wallet rejection, credentials precheck in _build_run_command, get-or-create topic label, and typed test helper). Verified the sibling Three new concerns:
🔗 cross-repo: This PR (ENGN-8646) adds Privy managed-custody worker lifecycle to the builder-kit, depending on allora-sdk-py's ForgeBackendClient.provision_wallet/clear_association and a deferred-managed AlloraWalletConfig.from_env(). The previously-flagged CRITICAL cross-repo findings (missing SDK methods, missing FORGE_SIGNING_WALLET_ID injection) are FALSE POSITIVES — they were raised against the wrong sibling branch (tani/ENGN-8615-sdk-privy-delegated-signing). The real dependency is allora-sdk-py #81 branch tani/engn-8646-auto-provision-managed-wallets, which the author verified implements both methods plus the deferred config, and the forge-v2 auto-provision branch adds the POST /api/v1/signing-wallets (GetOrCreateForTopic) and clear-association endpoints. The author also added real-SDK contract tests that lock the contract going forward. The one genuinely novel cross-repo gap I found that prior comments missed: FORGE_BACKEND_URL has a default (https://forge.allora.network) on the SDK side but is hard-required by the kit's WorkerManager, so an operator following the SDK README's API-key-only pattern cannot deploy a managed worker through the kit. Everything else is either covered and resolved, or a known prospective tracking item (go/ts managed-custody parity). No blocker. 🔬 deep-analysis: Managed-custody lifecycle changes are cohesive with migration handling and test coverage; no additional deep cross-file issues found beyond prior comments. ⚡ performance: The managed-custody additions are not algorithmically pathological (no O(n^2), no unbounded fan-out, no lock contention beyond the already-fixed _forge_client lazy build). The two perf-relevant patterns introduced by this diff are both on the managed-deploy/start path: (1) _deploy_managed_worker pays a Forge backend network round-trip on every deploy_worker(custody='managed') before consulting the local DB row that already stores the provisioned wallet id — medium severity because it serializes the operator deploy hot path behind a remote call returning data the manager already has; (2) _build_run_command copies os.environ per managed worker start, repeated N times during reconcile/start_all — low severity, avoidable per-startup allocation. Neither is a blocker; both have straightforward fixes. No DB N+1 or connection-pool concerns apply (SQLite, per-call connections are pre-existing and unchanged by this diff). 🔄 runtime-semantics: Managed-custody lifecycle changes appear runtime-safe: command/env construction, lazy client init, and start/remove behavior align with intended semantics; no new runtime-semantics defects identified beyond existing coverage. 🔒 security: Pass-1 security review of allora-forge-builder-kit#35 (managed-custody worker lifecycle). The two CRITICAL claims from prior passes (missing SDK methods on ForgeBackendClient, missing FORGE_SIGNING_WALLET_ID env injection) were verified as false positives against the wrong sibling branch — on the real dependency branch tani/engn-8646-auto-provision-managed-wallets, both provision_wallet and clear_association exist on the imported class, and _build_run_command DOES inject FORGE_SIGNING_WALLET_ID at line 578; AlloraWalletConfig.from_env() also defers without requiring it. The https-scheme concern (synth-017, resolved wontfix) was verified: the SDK's _validate_backend_url IS reached in the worker subprocess path via make_remote_wallet -> ForgeBackendClient.init. Three new gaps not covered by prior reviews: (1) MEDIUM — remove_worker deletes the local row before clear_association with no durable retry record, so a crash or transient backend failure orphans a live delegated-signing binding on the Forge backend with no local trace (root-cause distinct from the synth-004 exception-swallow framing); (2) LOW — _deploy_managed_worker always calls provision_wallet (API-key-bearing) before the existence check, amplified by no lock on deploy_worker; (3) INFO/cross-repo — operator-supplied topic_desc flows into the backend wallet 'label' persisted and rendered by forge-v2, a potential stored-XSS vector if forge-v2 does not escape labels at render time. SQL is parameterized throughout (sqlc-style ? placeholders in all sqlite3.execute calls). No secrets are logged. No path traversal (artifact paths are materialized under a uuid-named file in artifact_dir). No injection in subprocess argv (topic_id is int, address is bech32 from the backend). 🎨 style: The managed-custody additions are generally well-structured — the 📋 synthesis: PR#35 adds managed (Privy-via-Forge-backend) custody to WorkerManager. Many issues from prior passes have been fixed by the author (synth-001/002 FP on wrong branch, synth-004 exception swallow as product decision, synth-005 local-input rejection, synth-006 flag sync, synth-007 action label, synth-008/009 Protocol+Literal types, synth-010 identity_ref sentinel, synth-011/012 lock and ImportError message, synth-013 malformed wallet, synth-014 precheck, synth-016 label resolver, synth-017 helper typing). Phase A/B reviewers surfaced 29 new findings; after clustering and deduplication this pass yields 20 consolidated findings (4 dismissed). The key unresolved risks are: (HIGH) managed custody is architecturally complete but unreachable from the shipped workerctl CLI and web_dashboard — no operator can deploy or reconcile managed workers without writing Python directly; (MEDIUM) the allora-sdk>=1.0.6 lower bound admits published SDK versions without provision_wallet/clear_association, so pip-installed users hit AttributeError instead of the friendly error path; (MEDIUM) three durability gaps in the managed lifecycle — provision-before-write on deploy, DELETE-before-clear on remove, and silent signing_wallet_id overwrite on redeploy without releasing the prior wallet — can orphan backend bindings with no local retry record; and (MEDIUM) reconcile() has no terminal 'misconfigured' state, so a managed worker with missing credentials spins forever. Note: runtime-semantics and deep-analysis agents returned empty findings. Generated by code-review-forge — multi-agent PR review with temporal redundancy |
TaniBuilds
left a comment
TaniBuilds
left a comment
There was a problem hiding this comment.
🔍 Multi-Agent Inline Review
17 inline findings posted as resolvable threads.
Generated by code-review-forge
TaniBuilds
left a comment
TaniBuilds
left a comment
There was a problem hiding this comment.
🔍 Multi-Agent Inline Review
17 inline findings posted as resolvable threads.
1 findings not on changed lines (see PR comment for full report): 🟠 1 high
Generated by code-review-forge
| # Managed custody: release the (user, topic) binding on the backend so the topic slot is | ||
| # freed. Best-effort — the worker is already gone locally, and the backend get-or-create is | ||
| # idempotent, so a stale binding is simply reused on the next deploy rather than leaking. | ||
| if custody == "managed" and signing_wallet_id: |
There was a problem hiding this comment.
🟡 MEDIUM · 🔒 security · synth-004
remove_worker swallows all clear_association exceptions, masking permanently leaked signing-wallet bindings
The except Exception: block on lines 232–238 catches any failure from clear_association (auth errors, 403, transport failure) and logs only a warning. The worker row is already deleted before the call, so:
- If
clear_associationfails due to an expired or revokedFORGE_API_KEY, the local state shows the worker gone but the backend still holds a(user, topic) → wallet_idbinding that authorizes delegated signing indefinitely. - If the topic is decommissioned entirely and not re-deployed, the stale binding is never retried and remains live until manual cleanup.
- Auth failures (401/403) are indistinguishable from transient transport errors in the log, so operators have no signal to rotate the API key.
The comment claims 'a stale binding is simply reused on the next deploy rather than leaking' — true only if the same topic is re-deployed by the same operator; any other decommission leaves an orphaned signing authority.
💡 Suggestion: Persist failed clear_association attempts as a 'pending_release' record (topic_id, signing_wallet_id, last_error, attempted_at) in the DB so a reconcile loop or background janitor can retry. Distinguish auth failures (401/403) from transient errors and surface them as a distinct warning that the binding remains live and the API key may need rotation.
🔗 Cross-repo: affects allora-sdk-py, forge-v2
There was a problem hiding this comment.
Partly intentional, partly a real gap — leaving this open for a product decision.
Intentional: the broad best-effort catch is deliberate (stated in the PR description and the comment on the clear_association block in remove_worker): decommission must remove the worker locally regardless of backend reachability, and because the backend get-or-create is idempotent per (user, topic), a stale binding is reused on the next deploy of the same topic rather than leaking.
Real gap you've correctly flagged: for a topic that is permanently decommissioned (never re-deployed), the (user, topic)→wallet binding stays live until manual cleanup, and a 401/403 is logged indistinguishably from a transient error. A durable fix — a pending_release row + a reconcile/janitor retry, and elevating auth failures so operators know to rotate the key — needs a new table and background loop, which is a design decision beyond this managed-custody draft. Leaving unresolved to track that follow-up rather than silently closing it.
…er env (synth-002, synth-014) Pin the DB-stored signing_wallet_id into the managed subprocess env so the worker signs with the exact provisioned wallet (deterministic) rather than re-deriving one via topic get-or-create. Precheck the Forge api key / backend url / wallet id before spawning so a misconfigured managed worker fails loudly via start_worker/reconcile instead of the subprocess exiting right after the DB row is marked 'running'.
…001, synth-003) Add tests that exercise the actual allora_sdk surface the managed lifecycle depends on so a cross-repo contract break cannot hide behind the _FakeForgeClient stub or an argv-only check: - assert the real ForgeBackendClient exposes provision_wallet and clear_association - assert AlloraWalletConfig.from_env() defers (no crash) with only FORGE_API_KEY set - drive from_env() with the exact env _build_run_command injects and assert it builds a wallet-backed config without raising (HTTP mocked).
…) (synth-004, synth-006) status_worker() already returns custody and signing_wallet_id; mirror them in status_all() so consumers iterating the list (dashboards, CLIs) can distinguish managed from local workers without an N+1 per-worker round-trip.
…ynth-005, synth-007) deploy_worker(custody='managed', ...) silently dropped address/mnemonic/identity_alias. Raise a ValueError instead so an operator porting a local-deploy script doesn't silently orphan an address or leak a mnemonic for inputs the backend-provisioned managed path never uses.
…synth-006, synth-008, synth-018) _update_worker now writes reject_zero and signing_wallet_id when provided and returns the materialized artifact path. The managed redeploy path passes the new reject_zero and the freshly-provisioned wallet id (so a redeploy cannot leave a stale flag or wallet binding), and both replace paths use the returned path instead of re-reading via status_worker (drops one DB round-trip, synth-018). status_worker now also surfaces reject_zero so the spawned command actually reflects it.
…synth-009) A managed redeploy always rotates the artifact on the one-per-topic wallet, so returning action='reused' in auto mode was misleading for callers that branch on the action to fire notifications or downstream jobs. Always report 'replaced' when the artifact was rotated.
…-008, synth-012, synth-017) Replace forge_client: Optional[Any] and the untyped _forge_client() return with a ForgeClientProtocol (plus a ProvisionedWallet structural type for the provision result). This captures the two-method contract the managed lifecycle depends on, type-checks the injected client at the boundary without coupling to the concrete SDK class, and types the _managed_manager test helper's client parameter.
…ynth-009, synth-011) The custody discriminant is a fixed two-value set; annotate WorkerSpec.custody and deploy_worker(custody=...) with a CustodyMode = Literal["local", "managed"] alias for type-checker coverage and IDE completion while keeping it a plain str on the wire and in SQLite.
…10, synth-013) Managed workers have no identities row, so storing the Privy wallet UUID in identity_ref made the column masquerade as a local alias and duplicated signing_wallet_id without a coupling constraint. Use a 'managed' sentinel and keep signing_wallet_id as the canonical wallet id.
…ynth-011, synth-014) _forge_client() read-then-wrote self._forge_client_obj without the RLock every other state mutation uses, so two concurrent managed deploys could both construct a client and leak the loser's requests.Session. Build it inside `with self._lock:` (reentrant, so existing holders don't deadlock).
…nth-012, synth-015) The lazy `from allora_sdk.rpc_client.remote_signer import ForgeBackendClient` raised a raw ModuleNotFoundError when the optional SDK is absent, misleading users into thinking their API key was wrong. Catch ImportError and re-raise a clear "managed custody requires the allora-sdk package" message.
…-013, synth-016) _deploy_managed_worker used info.address / info.id without checking them. Raise RuntimeError if the backend returns an empty address or id so a malformed response fails loudly instead of inserting a broken worker row (which would skew _address_has_other_topics and pass a bad id to clear_association on removal).
…(synth-016-info) The managed wallet label fell back to 'worker-topic-N' even when the topic_desc_resolver could supply a real name. Route it through _resolve_topic_desc (the same source the rest of the registry uses) so the backend label is human-meaningful, keeping the topic_id fallback.
Automated-review triage — PR #35 (managed-custody worker lifecycle)Worked through all 34 open review threads (two review runs: 17 + 17). 32 resolved, 2 left open for a product decision. 13 commits pushed ( TL;DR on the two "criticals" — both are false positivesThe reviewer matched the wrong sibling SDK branch (
So the contract is satisfied end-to-end (kit → SDK → forge-v2). This remains a draft that can't merge until those stacked SDK/forge-v2 branches land, but the kit code is correct. A) Fixed (commit → what)
B) Declined, with reason (resolved)
C) Unresolved — need a product/coordination decision
D) Verification
|
TaniBuilds
left a comment
TaniBuilds
left a comment
There was a problem hiding this comment.
🔍 Multi-Agent Inline Review
16 inline findings posted as resolvable threads.
4 findings not on changed lines (see PR comment for full report): 🟠 1 high, 🟡 2 medium, 🔵 1 low
Generated by code-review-forge
| register a managed worker against its backend-assigned address. One wallet per topic, so a | ||
| re-deploy refreshes the artifact against the same wallet rather than allocating a new one. | ||
| """ | ||
| client = self._forge_client() |
There was a problem hiding this comment.
🟡 MEDIUM · 🔒 security · synth-p1-003
provision_wallet called before any local write — backend wallet binding is orphaned if artifact copy or DB INSERT fails
_deploy_managed_worker (worker_manager.py:482-534) executes:
- client.provision_wallet(topic_id, label=...) — backend now holds a (user, topic) → wallet binding
- _worker_exists check
- _update_worker or add_worker (which calls _materialize_artifact via shutil.copy2 + sqlite3 INSERT)
Step 3 can fail for many reasons: ENOSPC on shutil.copy2, sqlite3.OperationalError (database locked, disk full), sqlite3.IntegrityError from a concurrent deploy race, or SIGTERM/KeyboardInterrupt. When this happens, the backend binding survives (provision_wallet is idempotent at (user, topic) level, so a retry recovers the same wallet) BUT the operator only recovers the binding by re-running deploy_worker. If they instead run remove_worker to clean up, _get_custody returns ('local', None) (no row exists) and no clear_association is ever issued. The backend binding leaks permanently — the provisioned wallet_id was never persisted locally.
💡 Suggestion: Wrap the provision_wallet → add_worker sequence in a try/except: on any exception after a successful provision_wallet, log the provisioned wallet_id+address at WARN level (so an operator can grep logs) and best-effort call clear_association(info.id) to release the backend binding. Alternatively, stage a tombstone row (signing_wallet_id=NULL, custody='managed_provisioning') before the provision_wallet call, update it on success, and let reconcile() detect and clean up stale tombstones on restart.
🔗 Cross-repo: affects allora-sdk-py, forge-v2
| @@ -175,18 +257,34 @@ def add_worker(self, spec: WorkerSpec) -> None: | |||
| def remove_worker(self, topic_id: int, address: str, force: bool = False) -> None: | |||
There was a problem hiding this comment.
🟡 MEDIUM · 🔒 security · synth-p1-004
remove_worker deletes the local worker row BEFORE calling clear_association — a crash in this window orphans a live delegated-signing binding with no local retry record
remove_worker (worker_manager.py:257-279) executes:
- _get_custody reads signing_wallet_id into a local variable
- _archive_active_deployment
- DELETE FROM workers ... COMMIT
- clear_association(signing_wallet_id) ← only attempted after the row is gone
If the process is killed (SIGKILL, OOM, host reboot) between step 3 and step 4, the Forge backend retains an active (user, topic) → wallet_id binding that authorizes delegated Privy signing, while the local registry shows no worker and no pending-release record. There is no way to enumerate orphaned bindings from the kit; the backend binding leaks permanently.
This is distinct from the prior synth-004 concern (exception swallowing): even if clear_association succeeds, the window between DELETE and clear_association is non-retryable because signing_wallet_id is never persisted to a durable cleanup queue — it only exists as a local variable at that point.
Note: The author left synth-004 open as a 'product decision'; this finding adds the stronger ordering argument as a new angle.
| def remove_worker(self, topic_id: int, address: str, force: bool = False) -> None: | |
| def remove_worker(self, topic_id: int, address: str, force: bool = False) -> None: | |
| if force: | |
| self.stop_worker(topic_id, address) | |
| custody, signing_wallet_id = self._get_custody(topic_id, address) | |
| # For managed custody: release backend binding BEFORE local deletion so a crash | |
| # cannot orphan a live delegated-signing binding with no local retry record. | |
| if custody == "managed" and signing_wallet_id: | |
| try: | |
| self._forge_client().clear_association(signing_wallet_id) | |
| logger.info("released managed wallet %s topic binding (topic %s)", signing_wallet_id, topic_id) | |
| except Exception as e: # noqa: BLE001 | |
| logger.warning( | |
| "clear-association failed for managed wallet %s (topic %s): %s; removed locally anyway", | |
| signing_wallet_id, topic_id, e, | |
| ) | |
| self._archive_active_deployment(topic_id, address) | |
| with sqlite3.connect(self.db_path) as conn: | |
| conn.execute("DELETE FROM workers WHERE topic_id=? AND address=?", (topic_id, address)) | |
| conn.commit() | |
| self._monitor_disable(topic_id, address) |
🔗 Cross-repo: affects allora-sdk-py, forge-v2
| ) | ||
| address = info.address | ||
|
|
||
| if self._worker_exists(topic_id, address): |
There was a problem hiding this comment.
🟡 MEDIUM · 🔒 security · synth-p1-005
_update_worker silently overwrites signing_wallet_id on redeploy without releasing the prior wallet
In _deploy_managed_worker's redeploy path (lines 498-500), _update_worker is called with the freshly-provisioned info.id. _update_worker (worker_manager.py:996-1015) writes signing_wallet_id=COALESCE(?, signing_wallet_id) — when a non-NULL id is supplied, it overwrites the prior id with no diff check.
If the backend ever returns the same address but a different wallet_id for the same topic (wallet cleared/regenerated server-side, backend migration, DR event), _update_worker silently replaces the stored id. The previous wallet's backend binding is now invisible to the kit — clear_association can never be called on it because the id is gone, and no warning is logged. This is a reliance on a backend invariant (wallet_id stable per (user, topic)) that is not locally enforced.
While this backend invariant likely holds today, a future backend migration or incident could violate it silently, and the kit has no observability hook to detect the rotation.
💡 Suggestion: Before overwriting via _update_worker, read the current signing_wallet_id from the DB. If it is non-NULL and != info.id, log a WARN ('backend rotated wallet for topic {topic_id}: old={old_id} new={info.id}') and best-effort call clear_association(old_id) so the prior binding does not leak. Alternatively, refuse the redeploy and surface a clear error directing the operator to remove_worker + redeploy.
🔗 Cross-repo: affects allora-sdk-py, forge-v2
| # Imported lazily: local-custody installs need not import the SDK signing client. | ||
| try: | ||
| from allora_sdk.rpc_client.remote_signer import ForgeBackendClient | ||
| except ImportError as e: |
There was a problem hiding this comment.
🟡 MEDIUM · 🎨 style · synth-p1-007
ImportError is re-raised as ValueError in _forge_client() — misleads exception handlers and violates Python exception-type semantics
In _forge_client() (worker_manager.py:184-191), a missing optional dependency (allora_sdk) is caught as ImportError and re-raised as ValueError. This is semantically incorrect: ValueError conventionally signals that an argument has an invalid value, not that an optional package is missing. Callers writing except ImportError to detect missing-dependency situations will silently miss this exception; callers writing except ValueError for bad-argument handling will unexpectedly catch an import failure. The Allora Python style guide favours domain-specific exceptions; at minimum, re-raise as ImportError (preserving the chained cause) so the exception type communicates the root cause.
| except ImportError as e: | |
| except ImportError as e: | |
| raise ImportError( | |
| "managed custody requires the 'allora-sdk' package " | |
| "(allora_sdk.rpc_client.remote_signer.ForgeBackendClient); install it to deploy " | |
| "managed workers" | |
| ) from e |
| e, | ||
| ) | ||
|
|
||
| def status_worker(self, topic_id: int, address: str) -> dict: |
There was a problem hiding this comment.
🟡 MEDIUM · 🎨 style · synth-p1-008
status_worker and status_all use diverging magic column indices — reject_zero is at row[16] in one and row[14] in the other
The PR adds three new magic column accesses to status_worker (row[14]=custody, row[15]=signing_wallet_id, row[16]=reject_zero) and two to status_all (row[15]=custody, row[16]=signing_wallet_id). The reject_zero column lands at index 16 in status_worker but at index 14 in status_all because the two SELECT lists use different column orderings — a direct consequence of building the mapping by hand against positional indices.
The Allora Python style guide explicitly prefers structured types (dataclasses / TypedDict) over raw dicts with magic indices. A future column addition to either SELECT that misaligns the indices would be a silent wrong-value bug (no test failure, no type error). Using sqlite3.Row (name-based column access) or a WorkerStatus TypedDict would eliminate this fragility entirely.
💡 Suggestion: Set conn.row_factory = sqlite3.Row on the connection in both status_worker and status_all, then access columns by name (row['custody'], row['signing_wallet_id'], etc.) instead of index. Alternatively, define a WorkerStatus TypedDict and populate it from named fields to give callers static checks and IDE completion.
| @@ -47,6 +57,31 @@ class DeployResult: | |||
| message: str | |||
There was a problem hiding this comment.
🔵 LOW · 🎨 style · synth-p1-016
DeployResult.action typed as bare str with inline comment — should use Literal["created", "reused", "replaced"] for consistency with CustodyMode
The PR introduces CustodyMode = Literal["local", "managed"] specifically to give type-checker coverage on a fixed two-value discriminant. However, DeployResult.action (line 56) still uses bare str with a trailing comment # created|reused|replaced. This is exactly the pattern CustodyMode was added to replace. Defining action: Literal["created", "reused", "replaced"] would give callers static guarantees, allow exhaustiveness checks, and remove the comment. The inconsistency is especially visible because both are introduced/touched in the same PR.
| message: str | |
| action: Literal["created", "reused", "replaced"] |
| register a managed worker against its backend-assigned address. One wallet per topic, so a | ||
| re-deploy refreshes the artifact against the same wallet rather than allocating a new one. | ||
| """ | ||
| client = self._forge_client() |
There was a problem hiding this comment.
🔵 LOW · ⚡ performance · synth-p1-017
_deploy_managed_worker unconditionally calls provision_wallet before checking the local DB — redundant network round-trip on every redeploy
🤝 flagged by 2 agents: performance, security
_deploy_managed_worker unconditionally calls client.provision_wallet(topic_id, label=...) and only AFTERWARDS checks _worker_exists(topic_id, address). On the common redeploy path (same topic, refreshed artifact), a local DB row already exists with a stored signing_wallet_id and address. The backend call is idempotent but every managed redeploy pays a network round-trip to the Forge backend carrying FORGE_API_KEY even though the wallet binding is already stored locally.
Additionally, deploy_worker does not hold self._lock during this sequence, so two concurrent deploys for the same topic both call provision_wallet in parallel, doubling the API-key-bearing requests. The provision_wallet call is the only place the Forge API key leaves the process over the network, so minimizing its frequency reduces attack surface.
💡 Suggestion: Read the existing worker row for (topic_id) first; if a managed row with a non-null signing_wallet_id exists and replace=False, reuse the stored address/wallet_id and skip the provision_wallet call, calling the backend only when the row is missing or signing_wallet_id is null. If re-provisioning is required for correctness on every deploy, document it as an explicit contract and add a force_reprovision flag.
🔗 Cross-repo: affects allora-sdk-py, forge-v2
| "re-deploy with custody='managed' to provision the backend wallet" | ||
| ) | ||
| cmd.extend(["--custody", "managed"]) | ||
| env = os.environ.copy() |
There was a problem hiding this comment.
🔵 LOW · 🔒 security · synth-p1-018
_build_run_command copies the full parent process environment into managed worker subprocesses — principle-of-least-privilege violation
🤝 flagged by 2 agents: adversarial, performance
For managed workers, _build_run_command calls env = os.environ.copy() (line 573) on every invocation. This propagates EVERY environment variable the parent process carries — AWS_ACCESS_KEY_ID, DATABASE_URL, GITHUB_TOKEN, FORGE_OPERATOR_PRIVATE_KEY, etc. — into the managed subprocess, regardless of whether worker_runtime needs them. The blast radius is bounded (same codebase) but it is a least-privilege violation: a managed worker subprocess legitimately needs FORGE_API_KEY, FORGE_BACKEND_URL, FORGE_SIGNING_WALLET_ID, ALLORA_API_KEY, FEE_GRANTER, and standard infra vars (PATH, HOME).
Additionally, copying os.environ N times during start_all/reconcile for N managed workers is avoidable per-startup allocation (though low severity given typical N).
| env = os.environ.copy() | |
| cmd.extend(["--custody", "managed"]) | |
| allowlist = ("PATH", "HOME", "LANG", "LC_ALL", "PYTHONPATH", "PYTHONUNBUFFERED", "ALLORA_API_KEY", "FEE_GRANTER", "TZ") | |
| env = {k: v for k, v in os.environ.items() if k in allowlist} | |
| env["FORGE_API_KEY"] = self._forge_api_key | |
| env["FORGE_BACKEND_URL"] = self._forge_backend_url | |
| env["FORGE_SIGNING_WALLET_ID"] = signing_wallet_id |
| ) | ||
| cmd.extend(["--mnemonic-file", str(key_file)]) | ||
| env: Optional[dict[str, str]] = None | ||
| if status.get("custody") == "managed": |
There was a problem hiding this comment.
🔵 LOW · ✅ correctness · synth-p1-019
FEE_GRANTER is SDK-recommended for managed (Privy) signing but never plumbed by WorkerManager — operators get opaque tx-fee failures
The sibling allora-sdk-py's AlloraWalletConfig docstring explicitly states fee_granter 'is the recommended pairing for Privy-delegated (RemoteWallet) signing' — because a freshly-provisioned Privy wallet has zero ALLO and cannot pay gas fees without a granter. _build_run_command injects FORGE_API_KEY, FORGE_BACKEND_URL, FORGE_SIGNING_WALLET_ID — but not FEE_GRANTER. It falls through os.environ.copy(), so the SDK only sees a granter if the parent process happened to export it. There is no fee_granter parameter on WorkerManager.init and no precheck warns when managed custody is configured without a granter.
Failure mode: managed worker subprocess starts, RemoteWallet builds successfully, the first submission fails with an insufficient-fee error from the chain. reconcile() re-attempts forever (no terminal misconfigured state per synth-p1-006).
💡 Suggestion: Either (a) accept fee_granter on WorkerManager.init (defaulting to $FEE_GRANTER) and explicitly inject it into the managed env; or (b) emit logger.warning at deploy_worker(custody='managed') time when FEE_GRANTER is absent from the environment, naming the SDK recommendation. Add an equivalent precheck in build_run_command for parity with the existing FORGE* credential checks.
🔗 Cross-repo: affects allora-sdk-py
| self.provisioned: list[tuple[int, str | None]] = [] | ||
| self.cleared: list[str] = [] | ||
|
|
||
| def provision_wallet(self, topic_id: int, label: str | None = None): |
There was a problem hiding this comment.
🔵 LOW · 🎨 style · synth-p1-020
_FakeForgeClient.provision_wallet missing return type annotation in tests
The provision_wallet method in _FakeForgeClient (test file, line ~19) has parameter type hints but no return type annotation. The Allora Python style guide requires type hints on all functions. The return value is SimpleNamespace at runtime but should structurally satisfy ProvisionedWallet. Adding -> ProvisionedWallet (imported from worker_manager) would document the contract and let Pyright verify the test stub satisfies the Protocol.
| def provision_wallet(self, topic_id: int, label: str | None = None): | |
| def provision_wallet(self, topic_id: int, label: str | None = None) -> "ProvisionedWallet": |
1 participant
Summary
Builds the orchestrator-level managed-custody flow so the dev kit can run Privy-managed workers end-to-end — the deferred Phase-3 piece of ENGN-8646. Previously
WorkerManagerwas local-custody only (generated a mnemonic, persisted a key file, keyed everything by a local address); the--custody managedswitch only existed at the single-workerworker_runtimelayer.Stacked on #34 (the
--custody managedswitch + local SDK pin) and depends on allora-sdk-py #81 (provision_wallet+ the newclear_association).deploy_worker(custody="managed")provisions an idempotent get-or-create managed wallet bound to the topic via the Forge backend and registers the worker against the backend-assigned address — no local key. One wallet per topic (ENGN-8572); a re-deploy refreshes the artifact on the same wallet rather than allocating a new one.start_workerlaunches managed workers with--custody managedand injectsFORGE_API_KEY/FORGE_BACKEND_URLinto the subprocess env (no--mnemonic-file). Extracted into a testable_build_run_command(...).remove_workerreleases the(user, topic)binding viaclear-associationon decommission — best-effort: the worker is removed locally regardless, and the backend get-or-create is idempotent so a stale binding is simply reused next deploy.workerstable gainscustody+signing_wallet_id(with the existing lightweightALTER TABLEmigration pattern);status_workersurfaces them. The Forge client is built lazily fromFORGE_*env and is injectable for tests.The managed flow reuses
allora_sdk'sForgeBackendClientrather than re-implementing the (security-hardened) HTTP plumbing.Local testing
Inherits #34's
[tool.uv.sources]local SDK pin, so it runs against the local branch build now. Live run needs a forge-v2 backend (Privy auth key + owner) andFORGE_API_KEY/FORGE_BACKEND_URL.Test plan
tests/test_worker_manager.py— 15 pass (8 existing + 7 new): provision/register, idempotent redeploy, managed command/env construction, clear-on-remove, clear-failure tolerance, local-not-cleared, missing-credentials errorallora-sdk-py— 17 pass (addedtest_clear_association)Summary by cubic
Add managed custody to WorkerManager so the orchestrator can run Privy-managed workers end-to-end without local keys. Implements ENGN-8646 by provisioning one backend-managed wallet per (user, topic) and reusing it on redeploy.
New Features
deploy_worker(custody="managed")provisions an idempotent wallet via the Forge backend and registers the worker with the backend-assigned address; one wallet per topic; redeploy updates the artifact on the same wallet and syncsreject_zero/signing_wallet_id. Rejects local-only inputs (address/mnemonic/identity_alias) whencustody="managed".start_workeradds--custody managed, injectsFORGE_API_KEY/FORGE_BACKEND_URL/FORGE_SIGNING_WALLET_ID, and prechecks credentials and wallet id; command building extracted to_build_run_command(...).remove_workercalls backend clear-association to release the (user, topic) binding; best-effort (local removal always proceeds).workerstable addscustodyandsigning_wallet_id;status_workerandstatus_allreturn them.allora-sdkis missing, build the Forge client under the manager lock, type the client via a Protocol, useidentity_ref="managed"for managed rows, and reportaction="replaced"on managed redeploys.Migration
FORGE_API_KEYandFORGE_BACKEND_URLto use managed custody; otherwisedeploy_worker(..., custody="managed")fails fast.Written for commit 9aa5226. Summary will update on new commits.