refactor: centralize environment variable key constants#1730
refactor: centralize environment variable key constants#1730yinwm merged 3 commits intosipeed:mainfrom
Conversation
|
Hi, @alexhoshina |
|
hi @dev-miro26, please check the ci~ |
yinwm
left a comment
There was a problem hiding this comment.
Review Summary
Thanks for this refactoring PR! The intent is great — centralizing environment variable keys is a good practice to prevent typos and improve discoverability.
However, I found several issues that need to be addressed before merging.
🚨 Critical: Syntax Error (blocking CI)
File: pkg/migrate/sources/openclaw/openclaw_handler.go:6-8
In Go, import statements must appear before any other declarations. You placed a const before import:
package openclaw
const OpenclawHomeEnvVar = "OPENCLAW_HOME" // ❌ Wrong position
import ( // ❌ Syntax error: imports must appear before other declarations
...
)Fix: Move the const after the import block:
package openclaw
import (
"fmt"
...
)
const OpenclawHomeEnvVar = "OPENCLAW_HOME"This is causing all CI checks to fail (Linter, Security Check, Tests).
⚠️ Missing Replacement
File: web/backend/api/gateway.go:390
This line still uses the hardcoded string:
cmd.Env = append(cmd.Env, "PICOCLAW_CONFIG="+h.configPath)Should be changed to:
cmd.Env = append(cmd.Env, config.EnvConfig+"="+h.configPath)(Don't forget to add the import for github.com/sipeed/picoclaw/pkg/config if not already present)
💡 Suggestion: Add PICOCLAW_GATEWAY_HOST
The PICOCLAW_GATEWAY_HOST environment variable is used in:
pkg/config/config.go:628(struct tag)web/backend/api/gateway.go:393(string concatenation)
Consider adding it to pkg/config/envkeys.go for completeness:
// EnvGatewayHost overrides the host address for the gateway server.
// Default: "127.0.0.1"
EnvGatewayHost = "PICOCLAW_GATEWAY_HOST"📋 Checklist Before Merge
- Fix the syntax error in
openclaw_handler.go(move const after import) - Replace hardcoded string in
gateway.go:390 - Sign the CLA (currently pending) — visit: https://cla-assistant.io/sipeed/picoclaw?pullRequest=1730
- Ensure all CI checks pass (Linter, Security Check, Tests)
✅ What's Done Well
pkg/config/envkeys.gois well-documented with clear comments- Handling of circular import in
credential.gois pragmatic - Non-picoclaw env vars (
OPENCLAW_HOME,CODEX_HOME) placed in their domain packages — good DDD
Once the issues above are fixed, this will be a solid refactoring PR! 🦞
|
Hi, @alexhoshina @yinwm |
yinwm
left a comment
There was a problem hiding this comment.
Code Review: PR #1730
✅ Strengths
- Correct refactoring approach - Centralizing environment variable constants is a standard Go best practice
- Clear documentation - Comments in
pkg/config/envkeys.goare well-written, explaining each constant's purpose and default value - Precise scope - Pure refactoring with no functional changes
- Consistent style - Follows the existing pattern established by
credential.PassphraseEnvVar
⚠️ Points to Consider
1. Circular Import Technical Debt
pkg/credential/credential.go:26-29:
// picoclawHome is a package-local copy of config.EnvHome. It is kept here to
// avoid a circular import between pkg/credential and pkg/config.
const picoclawHome = "PICOCLAW_HOME"Issue: Duplicating the constant value creates potential for inconsistency. If someone updates config.EnvHome but forgets to sync this local copy, it would cause subtle bugs.
Suggestion: Consider placing these constants in a more foundational package (e.g., internal/envkeys or pkg/constants) that both config and credential can depend on without circular imports.
2. Missing Unit Tests (Minor)
While this is a pure refactor, it would be nice to have a simple test to verify constant values:
func TestEnvKeysMatchExpected(t *testing.T) {
tests := []struct {
constant, expected string
}{
{EnvHome, "PICOCLAW_HOME"},
{EnvConfig, "PICOCLAW_CONFIG"},
{EnvBuiltinSkills, "PICOCLAW_BUILTIN_SKILLS"},
{EnvBinary, "PICOCLAW_BINARY"},
{EnvGatewayHost, "PICOCLAW_GATEWAY_HOST"},
}
for _, tt := range tests {
if tt.constant != tt.expected {
t.Errorf("got %q, want %q", tt.constant, tt.expected)
}
}
}📋 Summary
| Item | Status |
|---|---|
| Code Quality | ✅ Good |
| Test Coverage | |
| Documentation | ✅ Clear |
| Risk | 🟢 Very Low |
Recommendation: Approve for merge. Consider addressing the circular import debt in a follow-up PR.
🤖 Generated with Claude Code
|
@dev-miro26 Nice cleanup! Replacing scattered inline env var strings with centralized constants across 11 files is exactly the kind of refactor that prevents silent bugs down the road. Follows the existing pattern from credential.PassphraseEnvVar too, so it feels consistent. We're running a PicoClaw Dev Group on Discord for contributors to connect. If you're interested, email |
Closes: #1638
📝 Description
Environment variable names for runtime configuration (e.g.
PICOCLAW_HOME,PICOCLAW_CONFIG,PICOCLAW_BUILTIN_SKILLS,PICOCLAW_BINARY) wereduplicated as inline string literals across 11 files. A single typo in any
one of them would silently read the wrong variable with no compiler warning.
This PR replaces every inline occurrence with a named exported constant,
making all supported runtime knobs discoverable in one place and preventing
future typos.
Summary of changes:
pkg/config/envkeys.gowith four exported constants (EnvHome,EnvConfig,EnvBuiltinSkills,EnvBinary)credential.SSHKeyPathEnvVar(was the unexportedsshKeyEnv);add
picoclawHomepackage-local constant to avoid a circular importopenclaw.OpenclawHomeEnvVarandproviders.CodexHomeEnvVarintheir respective domain packages
os.Getenv("PICOCLAW_*")call sites with the new constantsacross 9 files
🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
📚 Technical Context (Skip for Docs)
for preventing typos and improving discoverability. The existing
credential.PassphraseEnvVarwas already following this pattern;this PR extends it consistently to all picoclaw-specific runtime keys.
🧪 Test Environment
☑️ Checklist