feat: core improvements for local development - #480
Conversation
Task Queue Isolation: - Make task queue name configurable via TASK_QUEUE_NAME env var - Prevents workers from stealing tasks meant for other services Reshare Improvements: - Improved logging with request_party_id and local_party_prefix fields - Fixed party ID generation to use service's own LocalPartyPrefix config Bug Fixes: - Fix policy ID generation: check uuid.Nil instead of empty string - Auth middleware: validate token if provided, even when auth disabled Documentation: - Add worker-config.json example - Add LOCAL_DEPLOYMENT.md cheatsheet 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughThe changes modify authentication middleware control flow, UUID validation in policy creation, environment-aware queue naming for tasks, local party ID generation in vault service, and introduce a new worker configuration file with runtime settings for connectivity and operational parameters. Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom Pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vault/service.go (1)
1-255: Address gofmt formatting issue.The CI pipeline is reporting that this file is not properly formatted. Please run
gofmt -s -w vault/service.goto fix the formatting issue before merging.#!/bin/bash # Check formatting of vault/service.go gofmt -s -l vault/service.go
🤖 Fix all issues with AI agents
In @.run/LOCAL_DEPLOYMENT.md:
- Line 12: Update the LOCAL_DEPLOYMENT.md to clarify that the DYLD_LIBRARY_PATH
step is macOS-specific by adding a brief platform note near the existing export
line referencing DYLD_LIBRARY_PATH (and the darwin include path) — mark the
prerequisite as “macOS only” and add a short sentence advising Linux/Windows
users to use their OS-specific library/PATH setup or ensure go-wrappers build
artifacts are on their system PATH instead.
- Line 12: The file currently hard-codes developer-specific paths into
DYLD_LIBRARY_PATH (e.g., "/Users/dev/dev/vultisig/...") which will break other
environments; update the doc to instruct users to set a project root environment
variable (suggest VULTISIG_HOME) and show how to export DYLD_LIBRARY_PATH using
that variable (reference DYLD_LIBRARY_PATH and VULTISIG_HOME), or alternatively
add a prerequisites note telling developers to adjust those paths to their local
clone and demonstrate the relative path approach (e.g., use
$VULTISIG_HOME/go-wrappers/includes/darwin or ./go-wrappers/includes/darwin)
instead of the hard-coded absolute paths on lines 12 and 17.
- Around line 81-91: Update the local deployment docs to document the
TASK_QUEUE_NAME env var when running the worker: explain how to set
TASK_QUEUE_NAME alongside VS_WORKER_CONFIG_NAME when launching
cmd/worker/main.go (e.g., TASK_QUEUE_NAME=verifier-queue
VS_WORKER_CONFIG_NAME=worker-config go run cmd/worker/main.go) and add a note
about single-worker setups stating whether the default queue name (if any) is
sufficient or when a custom TASK_QUEUE_NAME must be used to avoid cross-service
task stealing; reference the TASK_QUEUE_NAME variable and the
VS_WORKER_CONFIG_NAME/ cmd/worker/main.go invocation so readers know where to
apply it.
🧹 Nitpick comments (2)
worker-config.json (1)
1-34: LGTM: Configuration file for local development.The configuration structure is appropriate for local development with sensible defaults. The static analysis warning about credentials in line 23 is a false positive—these are clearly example credentials for local development (myuser/mypassword, minioadmin/minioadmin).
To prevent accidental production use, consider adding a comment at the top of the file indicating this is for local development only.
📝 Optional: Add documentation header
{ + "_comment": "This configuration is for local development only. Do not use in production.", "log_format": "text", "vault_service": {plugin/tasks/tasks.go (1)
10-19: LGTM: Task queue isolation enabled.The environment-based queue name configuration successfully enables running multiple workers with isolated task queues, which aligns with the PR objectives. The implementation:
- Reads
TASK_QUEUE_NAMEenvironment variable for flexibility- Falls back to
defaultQueueNamefor backward compatibility- Is determined at initialization time
One minor consideration: Since
QUEUE_NAMEis an exported variable rather than a constant, it's technically modifiable at runtime (though unlikely in practice). If immutability is important, consider making it a function or using an unexported variable with an exported getter.Based on learnings, this queue name is correctly separated from infrastructure references like database/bucket names.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.run/LOCAL_DEPLOYMENT.mdinternal/api/middleware.gointernal/api/policy.goplugin/tasks/tasks.govault/service.goworker-config.json
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: evgensheff
Repo: vultisig/verifier PR: 286
File: plugin/tasks/tasks.go:9-9
Timestamp: 2025-07-31T09:12:18.809Z
Learning: In the Vultisig verifier project, "vultisig-verifier" appears in configuration files, docker-compose.yaml, and test files as infrastructure references (PostgreSQL database names, S3/MinIO bucket names, Docker environment variables) and should not be confused with the QUEUE_NAME constant in plugin/tasks/tasks.go which specifically refers to the Asynq task queue name. These infrastructure references should remain unchanged when queue names are modified.
📚 Learning: 2025-05-26T00:35:50.407Z
Learnt from: RaghavSood
Repo: vultisig/verifier PR: 87
File: internal/api/plugin.go:0-0
Timestamp: 2025-05-26T00:35:50.407Z
Learning: Policy verification logic in internal/api/plugin.go will be refactored when recipe workflows are applied to policy verification in a subsequent PR.
Applied to files:
internal/api/policy.go
📚 Learning: 2025-07-24T11:10:49.405Z
Learnt from: garry-sharp
Repo: vultisig/verifier PR: 274
File: internal/storage/postgres/policy.go:304-304
Timestamp: 2025-07-24T11:10:49.405Z
Learning: In internal/storage/postgres/policy.go, the DeletePluginPolicySync method intentionally updates the plugin_policies table to set deleted=true rather than deleting from plugin_policy_sync table. This is the correct behavior as confirmed by garry-sharp in PR #274.
Applied to files:
internal/api/policy.go
📚 Learning: 2025-06-10T18:57:04.359Z
Learnt from: webpiratt
Repo: vultisig/verifier PR: 156
File: internal/api/plugin.go:72-86
Timestamp: 2025-06-10T18:57:04.359Z
Learning: In internal/api/plugin.go (SignPluginMessages), the correct workflow is to persist the transaction via txIndexerService.CreateTx *before* policy evaluation, leaving it in the PROPOSED state; this behavior is intentional and should not be treated as an issue.
Applied to files:
internal/api/policy.go
📚 Learning: 2025-07-09T22:25:01.536Z
Learnt from: webpiratt
Repo: vultisig/verifier PR: 240
File: internal/api/policy.go:72-96
Timestamp: 2025-07-09T22:25:01.536Z
Learning: In internal/api/policy.go, the commented-out block for mocking required fields (lines 72-96) is intentionally kept as temporary code until UI fixes are completed, as confirmed by webpiratt. This pattern is acceptable in this specific context.
Applied to files:
internal/api/policy.go
📚 Learning: 2025-07-31T09:12:18.809Z
Learnt from: evgensheff
Repo: vultisig/verifier PR: 286
File: plugin/tasks/tasks.go:9-9
Timestamp: 2025-07-31T09:12:18.809Z
Learning: In the Vultisig verifier project, "vultisig-verifier" appears in configuration files, docker-compose.yaml, and test files as infrastructure references (PostgreSQL database names, S3/MinIO bucket names, Docker environment variables) and should not be confused with the QUEUE_NAME constant in plugin/tasks/tasks.go which specifically refers to the Asynq task queue name. These infrastructure references should remain unchanged when queue names are modified.
Applied to files:
plugin/tasks/tasks.go
🧬 Code graph analysis (1)
internal/api/middleware.go (1)
plugin/server/auth.go (1)
Auth(11-13)
🪛 Checkov (3.2.334)
worker-config.json
[medium] 23-24: Basic Auth Credentials
(CKV_SECRET_4)
🪛 GitHub Actions: Deploy Verifier Server
vault/service.go
[error] 1-1: Code is not formatted properly: gofmt -s -l . reported unformatted file(s). Run 'gofmt -s -w' to fix.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (5)
internal/api/policy.go (1)
71-73: LGTM: Improved UUID validation.Using
uuid.Nilfor checking unset UUIDs is more idiomatic and efficient than converting to string and comparing to empty string. This change improves code clarity and correctness.internal/api/middleware.go (1)
41-47: LGTM: Security improvement for auth validation.The updated logic ensures that when a token is provided, it's always validated even when authentication is disabled. This prevents potential security bypasses where invalid tokens could be accepted simply because auth is disabled. The behavior is now:
- No token + auth disabled → proceed
- No token + auth enabled → 401
- Token provided → always validate (regardless of auth setting)
This aligns with the security fix mentioned in the PR objectives.
vault/service.go (2)
211-217: LGTM: Improved logging clarity.The renamed logging field from "local_party_id" to "request_party_id" and the addition of "local_party_prefix" make it clearer which party ID is from the request versus the service's own configuration. This improves observability for reshare operations.
227-235: LGTM: Correct party ID generation using service's own prefix.The fix ensures the service generates its local party ID using its own configured
LocalPartyPrefixrather than accepting a prefix from the request. This is critical for proper task queue isolation and prevents clients from controlling the service's party identification.The comment clearly explains the rationale for this change.
plugin/tasks/tasks.go (1)
30-30: Correct usage of dynamic queue name.The
GetTaskResultfunction correctly uses the dynamically determinedQUEUE_NAMEvariable, ensuring it queries the appropriate queue based on the environment configuration.
| docker network create shared_network | ||
|
|
||
| # Set library path (required for TSS operations) | ||
| export DYLD_LIBRARY_PATH=/Users/dev/dev/vultisig/go-wrappers/includes/darwin/:$DYLD_LIBRARY_PATH |
There was a problem hiding this comment.
Clarify that DYLD_LIBRARY_PATH setup is macOS-only.
The DYLD_LIBRARY_PATH environment variable on Line 12 is specific to macOS. Linux and Windows developers will not need this step. Add a platform note to prevent confusion.
🔎 Suggested clarification
# Prerequisites (macOS only)
```bash
# macOS: Set library path (required for TSS operations)
export DYLD_LIBRARY_PATH=/Users/dev/dev/vultisig/go-wrappers/includes/darwin/:$DYLD_LIBRARY_PATHFor Linux and Windows, ensure the go-wrappers build artifacts are in your system PATH or library path as appropriate for your OS.
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @.run/LOCAL_DEPLOYMENT.md at line 12, Update the LOCAL_DEPLOYMENT.md to
clarify that the DYLD_LIBRARY_PATH step is macOS-specific by adding a brief
platform note near the existing export line referencing DYLD_LIBRARY_PATH (and
the darwin include path) — mark the prerequisite as “macOS only” and add a short
sentence advising Linux/Windows users to use their OS-specific library/PATH
setup or ensure go-wrappers build artifacts are on their system PATH instead.
</details>
<!-- fingerprinting:phantom:triton:mongoose -->
---
_⚠️ Potential issue_ | _🟡 Minor_
**Replace hard-coded developer paths with instructions for setup.**
Lines 12 and 17 contain hard-coded paths (`/Users/dev/dev/vultisig/...`) that are specific to one developer's machine. This will require manual edits for other developers. Provide instructions to set this up generically, for example via an environment variable or by describing the relative directory structure.
<details>
<summary>🔎 Suggested approach</summary>
```bash
# Example: guide users to set VULTISIG_HOME or similar
export VULTISIG_HOME=~/path/to/vultisig # or wherever they cloned the repos
export DYLD_LIBRARY_PATH=$VULTISIG_HOME/go-wrappers/includes/darwin:$DYLD_LIBRARY_PATH
cd $VULTISIG_HOME/verifier
Alternatively, note in the prerequisites section that developers must adjust paths for their environment.
Also applies to: 17-17
🤖 Prompt for AI Agents
In @.run/LOCAL_DEPLOYMENT.md at line 12, The file currently hard-codes
developer-specific paths into DYLD_LIBRARY_PATH (e.g.,
"/Users/dev/dev/vultisig/...") which will break other environments; update the
doc to instruct users to set a project root environment variable (suggest
VULTISIG_HOME) and show how to export DYLD_LIBRARY_PATH using that variable
(reference DYLD_LIBRARY_PATH and VULTISIG_HOME), or alternatively add a
prerequisites note telling developers to adjust those paths to their local clone
and demonstrate the relative path approach (e.g., use
$VULTISIG_HOME/go-wrappers/includes/darwin or ./go-wrappers/includes/darwin)
instead of the hard-coded absolute paths on lines 12 and 17.
- Clarify DYLD_LIBRARY_PATH is macOS-only - Add TASK_QUEUE_NAME documentation for multi-worker setups 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Info already exists in devenv/README.md and CLAUDE.md 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
gomesalexandre
left a comment
gomesalexandre
left a comment
There was a problem hiding this comment.
CI green. Local dev improvements - no prod code changes. LGTM.
| @@ -0,0 +1,34 @@ | |||
| { | |||
There was a problem hiding this comment.
should-fix: worker-config.json is committed to the repo with embedded credentials
Even though these are local dev creds (test123, minioadmin, mypassword), the .gitignore already covers worker_config.json (underscore) and config.json but NOT worker-config.json (hyphen). This file will land in git history.
Two options:
- rename to
worker_config.jsonso it's gitignored automatically - add
worker-config.jsonto .gitignore and move a sanitized template to a docs folder
Probably want option 1 for zero friction.
There was a problem hiding this comment.
@jpthor can you confirm these are fixture-only local dev creds and will never appear in a prod config? if so we can dismiss the thread — just need explicit confirmation before merge so future devs reading the history know it was intentional.
|
auto-merge is enabled (squash) but blocked on unresolved review threads. @jpthor — 3 threads need resolving before merge fires: the coderabbitai DYLD_LIBRARY_PATH callout, the TASK_QUEUE_NAME doc gap, and the worker-config.json credentials note I left. Resolve those and auto-merge should kick. |
gomesalexandre
left a comment
gomesalexandre
left a comment
There was a problem hiding this comment.
review
worker-config.json - the big one
The file has all placeholder / local-dev values:
"encryption_secret": "test123"- obviously not a real secret"access_key": "minioadmin"/"secret": "minioadmin"- minio defaults, well-known defaults shipped with minio docker"dsn": "postgres://myuser:mypassword@localhost:5432/..."- localhost postgres with generic credsrelay.serverpoints toapi.vultisig.com/routerwhich is a public relay endpoint, no secret
None of these are real credentials. Safe to merge from a secrets standpoint.
That said - there is a .gitignore gap worth addressing:
preferably-blocking: .gitignore already ignores worker.json and worker_config.json (underscore) but misses worker-config.json (hyphen). The intent is clearly to keep these local-only config files out of the repo. Either this file should be renamed to worker_config.json to match the gitignore pattern, OR worker-config.json should be added to .gitignore and this file should become a worker-config.example.json template. As-is, any real secrets dropped into worker-config.json by a future dev would silently get committed. The gitignore hole is the actual problem here, not the current content.
middleware.go - auth logic
The change is a genuine security improvement. Old code:
if auth disabled { skip all validation }
New code:
if no token AND auth disabled { skip }
if no token AND auth enabled { 401 }
if token present { always validate }
This correctly closes the case where someone passes a token with auth disabled - previously the token was never validated. Now it always gets validated if present. Good catch.
suggestion: The log message "Auth is disabled and no token provided" at Info level is potentially chatty - every unauthenticated request in local dev will spam the log. Could be Debug level.
policy.go - uuid.Nil check
uuid.UUID is [16]byte. uuid.UUID{}.String() returns "00000000-0000-0000-0000-000000000000" (36 chars), never empty string. So the old check policy.ID.String() == "" was dead code that never fired - any unset UUID would not get a fresh ID generated for it. The uuid.Nil check is the correct fix. Clean.
tasks.go - QUEUE_NAME env var
QUEUE_NAME changed from const to var initialized at package load time via os.Getenv. This is fine for an env-driven value, but worth noting: since it's a package-level var evaluated once at startup, changing TASK_QUEUE_NAME after process start won't take effect (expected behaviour, not a bug). The exported var QUEUE_NAME is used in cmd/worker/main.go for queue routing so this threads through correctly.
suggestion: The exported name QUEUE_NAME is SCREAMING_CASE which is a Go anti-pattern for vars (only consts use that style idiomatically). Would be cleaner as QueueName. Low stakes but it's a Go convention thing.
vault/service.go - reshare localPartyID change
Old: vcommon.GenerateLocalPartyId(s.cfg.LocalPartyPrefix) - returns {prefix}-{random 4-digit int} (1000-9999 range)
New: s.cfg.LocalPartyPrefix + "-" + req.SessionID[:8] - deterministic, based on first 8 chars of sessionID
preferably-blocking: The new ID is deterministic per session prefix. GenerateLocalPartyId used a random 4-digit suffix specifically to avoid collisions when the same service registers for multiple sessions simultaneously. With the new scheme: if two reshare tasks for the same LocalPartyPrefix arrive while both have a SessionID that starts with the same 8 chars, they'd get the same LocalPartyId. SessionIDs are presumably UUIDs (random), so first-8-char collisions are unlikely but non-zero. More importantly - the original randomness was intentional to ensure uniqueness at the relay registration level. The PR description says this fixes party ID to use "service's own LocalPartyPrefix config" which is correct, but that fix could be achieved by just passing s.cfg.LocalPartyPrefix to the existing GenerateLocalPartyId (which is what the old code already did). I'm not seeing the bug that necessitated switching from random to deterministic. What was wrong with vcommon.GenerateLocalPartyId(s.cfg.LocalPartyPrefix)? If the old code was passing req.LocalPartyId as the prefix instead of s.cfg.LocalPartyPrefix, that's a different fix - but looking at the original code: vcommon.GenerateLocalPartyId(s.cfg.LocalPartyPrefix) - it already used s.cfg.LocalPartyPrefix. So what exactly is being fixed here?
q: Can you clarify what was broken with the original vcommon.GenerateLocalPartyId(s.cfg.LocalPartyPrefix) call that this deterministic scheme fixes? The commit says "Fixed party ID generation to use service's own LocalPartyPrefix config (not the one from request)" but s.cfg.LocalPartyPrefix was already being used in the old code per the diff. Not seeing the bug.
verdict
REQUEST_CHANGES - the preferably-blocking findings need answers before merge:
worker-config.jsongitignore gap needs fixing (rename to match existing pattern, or add to gitignore + make.example)- The localPartyID determinism change needs justification - if there's a real bug being fixed, great, but the reasoning in the PR description doesn't match the diff
The auth middleware and uuid.Nil fixes are solid.
2 participants
Summary
Core improvements extracted from local development work. No devctl/devenv CLI - those live in vultisig-cluster.
Task Queue Isolation
TASK_QUEUE_NAMEenv varReshare Improvements
request_party_idandlocal_party_prefixfieldsLocalPartyPrefixconfig (not the one from request)Bug Fixes
uuid.Nilinstead of empty stringDocumentation
worker-config.jsonexample for local developmentLOCAL_DEPLOYMENT.mdcheatsheetTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.