feat(phase-06): make secrets specific to installed claws, not global#16
Merged
feat(phase-06): make secrets specific to installed claws, not global#16
Conversation
Plan breakdown: - 06-01: Refactor core secrets module for per-instance scoping - 06-02: Update CLI commands for per-claw secrets - 06-03: Update status display for degraded state Requirements: PSEC-01 through PSEC-05 Decisions implemented: - D-01 through D-06: Per-claw-instance secrets with host:claw_type:claw_name identity - D-07 through D-09: Nested JSON storage structure - D-10 through D-13: CLI command changes - D-14 through D-17: Install flow changes (degraded state) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add tests for get_instance_key, get_instance_secrets, set_instance_secret - Add tests for remove_instance_secret, list_instances_with_secrets - Test same key different values per instance - Update existing tests to use nested structure - Remove deprecated global function tests TDD RED phase - tests fail as expected
- Refactor load_secrets/save_secrets to use nested structure: {instance_key: {secret_key: SecretEntry}}
- Add get_instance_key(host, claw_type, claw_name) -> instance key formatter
- Add get_instance_secrets(instance_key) -> dict of secrets for instance
- Add set_instance_secret(instance_key, key, value, description) -> per-instance set
- Add remove_instance_secret(instance_key, key) -> per-instance remove
- Add list_instances_with_secrets() -> list of instance keys with secrets
- Add ClawNotFoundError exception class
- Keep legacy global functions (get_secret, set_secret, etc.) using __global__ namespace for CLI backward compatibility
- Update CLI secret list to read from __global__ namespace
- Update test assertions to check __global__ namespace
- File permissions remain 0o600
- Same locking and atomic write patterns
TDD GREEN phase - all tests pass
Addresses: PSEC-01, PSEC-05
- Add ClawStatus.DEGRADED enum value for running claws with missing secrets - Add missing_secrets field to HealthResult TypedDict - Implement get_missing_secrets() helper to check required secrets - Update check_claw_health() to return DEGRADED when missing required secrets - Update all return statements to include missing_secrets field - Add comprehensive tests for degraded state and secret checking - Running claws with all secrets show as RUNNING, not DEGRADED Fixes PSEC-04 health check requirement.
- Modified set_cmd to require claw_name as first positional argument - Added get_installed_claw() function to validate claw exists in hosts - Updated set_cmd to use get_instance_key and set_instance_secret - Validates claw exists before accepting secrets (raises ClawNotFoundError) - Updated all existing set tests to use per-claw pattern with hosts - Added new tests: test_secret_set_with_claw, test_secret_set_claw_not_found Changes: - src/clawrium/core/secrets.py: Added get_installed_claw() function - src/clawrium/cli/secret.py: Updated set_cmd signature and implementation - tests/test_cli_secret.py: Updated all set tests for per-claw pattern All set tests passing. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Import HealthResult to store full health check results - Change health_results dict to store HealthResult instead of just ClawStatus - Add DEGRADED status display with yellow color - Show missing secret keys in degraded message (first 3, truncate rest) - Update all existing tests to include missing_secrets field in mocks - Add tests for degraded state display and truncation Running claws with missing secrets now show: degraded (missing: KEY1, KEY2, KEY3) Completes PSEC-04 status display requirement.
- Modified list_cmd to show secrets grouped by claw instance - list_cmd iterates over hosts and their installed claws - Shows missing required secrets per claw instance - Shows "No claws installed" when hosts.json is empty - Modified remove_cmd to require claw_name as first positional argument - remove_cmd validates claw exists before removing secrets - Updated all existing list and remove tests for per-claw pattern - Added new tests: test_secret_list_grouped_by_claw, test_secret_list_shows_missing_required, test_secret_list_no_claws_installed, test_secret_remove_with_claw, test_secret_remove_claw_not_found Changes: - src/clawrium/cli/secret.py: Updated list_cmd and remove_cmd implementations - tests/test_cli_secret.py: Updated all list/remove tests for per-claw pattern All 273 tests passing. Linter passing. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
B1: Validate instance key components reject colons to prevent
namespace spoofing. Added INSTANCE_KEY_COMPONENT_PATTERN and
InvalidInstanceKeyComponentError.
B2: Add shlex.quote() defense-in-depth for claw_user in shell
command despite regex validation.
B3: Extract _save_secrets_atomic() helper to eliminate 4x
duplicated atomic save blocks.
B4/B5: Add comprehensive tests for get_missing_secrets() with
instance_key verification. Tests cover: name field priority,
user fallback derivation, multi-hyphen names, no-hyphen
fallback, empty/missing fields.
W1: Use claw_record['name'] directly instead of fragile
derivation from claw_user field.
Tests: 287 passed (14 new tests added)
ATX Review Summary
Review 1: Rating 3/5
Blocking issues:
| # | Location | Issue | Status |
|---|----------|-------|--------|
| B1 | secrets.py:180-191 | Instance key injection via colon | FIXED |
| B2 | health.py:155 | Shell injection risk | FIXED |
| B3 | secrets.py (4 locations) | Duplicated save logic | FIXED |
| B4 | get_missing_secrets() | Zero tests for core logic | FIXED |
| B5 | test_health.py:224-250 | Instance key not verified | FIXED |
Warnings addressed:
| # | Issue | Status |
|---|-------|--------|
| W1 | Fragile claw name derivation | FIXED |
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: @atx-ci <269048218+atx-ci@users.noreply.github.com>
B1: Extract hosts_with_installed_claw fixture to conftest.py.
Refactored test_cli_secret.py to use shared fixture, eliminating
12+ duplicate host-setup blocks.
B2: Add missing_secrets assertion to test_health_check_stopped.
STOPPED status should not check secrets (missing_secrets = None).
Tests: 287 passed
ATX Review Summary
Review 2: Rating 3/5
Blocking issues:
| # | Location | Issue | Status |
|---|----------|-------|--------|
| B1 | test_cli_secret.py | 12+ duplicate host-setup blocks | FIXED |
| B2 | test_health.py:53 | Missing assertion for missing_secrets | FIXED |
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: @atx-ci <269048218+atx-ci@users.noreply.github.com>
- Make secret list require claw_name argument (no global listing) - Add SecretsFileCorruptedError handling to set_cmd and remove_cmd - Expand get_installed_claw to search name, user, and claw_type fields - Inject secrets as ansible vars during install - Add systemd service creation and startup to zeroclaw/openclaw playbooks - Add LLM config (provider_url, model, api_key) to zeroclaw playbook - Declare LLM secrets in zeroclaw manifest - Add tests for invalid key format, corrupted secrets, keyboard interrupt ATX Review Summary Review 1: Rating 3/5 Blocking issues fixed: B1-B4 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
host:claw_type:claw_nameRequirements Delivered
Test plan
ATX Review Summary
Review 1: Rating 3/5
Blocking issues:
secrets.py:180-191get_instance_key()concatenates components with:without validation. Hostname containing:spoofs another instance's namespace.:. Reuse/extendKEY_ID_PATTERN.health.py:155check_cmdbuilds shell command withf'pgrep -u {claw_user} node ...'. Shell injection risk if regex weakened.shlex.quote(claw_user)defense-in-depth.secrets.py(4 locations)_save_secrets_locked()helper.get_missing_secrets()split('-', 1)parsing never exercised.tests/test_health.py:224-250instance_keyargument passed to mockedget_instance_secrets.call_argsand assert instance key matches.Warnings:
health.py:57-62claw_nameexplicitly fromclaw_record['name']. ✅ FIXEDsecrets.py:365-524DeprecationWarning.secrets.py:145save_secrets()exported but doesn't acquire lock internally.Review 2: Rating 3/5
Blocking issues:
tests/test_cli_secret.pyhosts_with_one_clawfixture toconftest.py.tests/test_health.py:53test_health_check_stoppeddoesn't assertresult['missing_secrets'] is None. No guard against regressions.Warnings:
secrets.py:129-133from Noneto suppress chain.secrets.py:394-526DeprecationWarning.Suggestions:
time.sleep(0.1)with monkeypatched datetimeCo-Authored-By: @atx-ci 269048218+atx-ci@users.noreply.github.com
🤖 Generated with Claude Code