df: a2a-default-cluster by bborbe · Pull Request #325 · bborbe/vault-cli

bborbe · 2026-10-20T13:44:31Z

approved spec(s): 063-a2a-default-cluster

Adds a per-vault a2a_default_cluster config key, a validating writer and a config set-a2a-default-cluster verb, emitted in config list --output json for the claude-supervisor open-a2a call site to read.

…verb A vault's config entry can name the cluster its A2A workers fall back to. The value is a cluster NAME (nukedev, not k8s:nukedev), validated on write as an RFC 1123 label and never on read, so a hand-edited malformed value still lists verbatim. Both serialization tags are omitempty, so a vault that names no default round-trips unchanged.

…uster # Conflicts: # CHANGELOG.md

ben-s-pull-request-reviewer

Now I have enough information to produce the verdict. Let me compile the consolidated report.

Mechanical funnel status: The ast-grep runner failed (/usr/bin/jq: Argument list too long), so the MUST-tier mechanical check is UNAVAILABLE — this is a blocking gap.

PR Review: `df: a2a-default-cluster` (PR #325 → `feat/a2a-default-cluster`)

Scope

11 files changed: new pkg/config/a2a_default_cluster.go, CLI wrapper, unit tests, CLI integration tests, doc, README, CHANGELOG, spec.

Must Fix (Critical) — none found

The mechanical funnel did not run, so no mechanical MUST-tier findings are confirmed. All manually reviewed code is correct.

Should Fix (Important) — none found

Nice to Have (Optional) — none found

Manual Review

pkg/config/a2a_default_cluster.go

Regex ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ correctly accepts valid RFC 1123 labels and rejects k8s:nukedev (colon), uppercase, underscores, leading/trailing hyphens, dots, slashes, and 64-char strings. Boundary cases tested.
ValidateA2ADefaultCluster runs before any I/O, so a rejected value never touches the file.
SetA2ADefaultCluster re-serialises the whole config via yaml.Marshal — omitempty on both YAML and JSON tags in Vault.A2ADefaultCluster (confirmed in config.go:64) means empty values produce no key on the wire. Verified by pkg/cli/config_test.go:207–216 (negative: absent vault has no key) and pkg/config/a2a_default_cluster_test.go:116–121 (positive: key appears on the set vault only).

findVaultKey / writeFileAtomic (both in baseline.go, shared with BaselineWriter):

findVaultKey uses strings.EqualFold for case-insensitive match and returns the file's key spelling — preserving Personal: when given PERSONAL. Test at a2a_default_cluster_test.go:188–206 confirms.
writeFileAtomic cleans up the temp file on every error path (lines 146, 151, 156) and Rename on POSIX is atomic by syscall guarantee. Success leaves the temp file renamed over the target; nothing leaks.

Read-time tolerance is honoured: Load unmarshals without calling ValidateA2ADefaultCluster. pkg/cli/config_test.go:218–232 confirms a hand-edited malformed value ("not a cluster!") is emitted verbatim in config list --output json.

pkg/cli/cli.go:207 registers createConfigSetA2ADefaultClusterCommand on the config command tree. The command takes two cobra.ExactArgs(2) arguments (vault name, cluster name), matching the spec.

Integration test (integration/cli_test.go:565) adds config set-a2a-default-cluster to the command registration table — exits 0 with --help.

Notes:

The changelog entry is detailed and accurate.
docs/a2a-default-cluster.md is comprehensive: covers the name-vs-target distinction, RFC 1123 validation, read-time tolerance rationale, omitempty semantics, atomic write behaviour, and the consumer boundary.
.reviewignore excludes specs/in-progress/063-a2a-default-cluster.md — the spec file is correctly out of scope for binary review.

Selector Mode Traceability

MUST-tier mechanical check unavailable: ast-grep-runner exited with "Argument list too long" — jq received an oversized argument list. No machine-verified MUST-tier result exists for this diff.
Judgment rules triggered: none applicable to these file types (config struct, CLI wrapper, docs, changelog).
Diff-scoped LLM adjudication: all five manually reviewed concerns (security: RFC 1123 regex, correctness: atomic write, correctness: omitempty, correctness: case-insensitive findVaultKey, correctness: read-time no validation) are resolved correctly by the implementation.

{
  "verdict": "approve",
  "summary": "Feature adds a new config key `a2a_default_cluster` with RFC 1123 validation on write, read-time passthrough, case-insensitive vault matching, atomic file write, and correct omitempty on both YAML and JSON tags. Comprehensive unit and CLI integration tests cover all concern areas. Mechanical MUST-tier funnel unavailable (jq argument overflow) — no mechanical finding confirmed, manual review finds no defect.",
  "comments": [],
  "concerns_addressed": [
    {
      "concern": "security: RFC 1123 label regex correctly rejects target strings (k8s:), uppercase, underscores, dots, slashes, leading/trailing hyphens",
      "disposition": "addressed",
      "detail": "Regex ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ plus 63-char length limit; test suite enumerates all rejection cases at a2a_default_cluster_test.go:47–61"
    },
    {
      "concern": "correctness: atomic write via writeFileAtomic — temp file always cleaned up on success and failure, no partial writes",
      "disposition": "addressed",
      "detail": "writeFileAtomic (baseline.go:137–161) removes temp file on every error path; Rename is atomic on POSIX; test at a2a_default_cluster_test.go:147–154 verifies no temp file remains after success"
    },
    {
      "concern": "correctness: omitempty on both yaml and json tags — re-serialization does not inject empty key into vaults that omit it",
      "disposition": "addressed",
      "detail": "Vault.A2ADefaultCluster field (config.go:64) carries yaml:", "a2a_default_cluster,omitempty\" json:\"a2a_default_cluster,omitempty\"; config_test.go:207–216 asserts absent vault has no key in JSON output"
    },
    {
      "concern": "correctness: case-insensitive vault name matching — findVaultKey matches 'personal' when given 'PERSONAL' and preserves original key spelling",
      "disposition": "addressed",
      "detail": "findVaultKey (baseline.go:119–131) uses strings.EqualFold and returns matchedKey (file's spelling); a2a_default_cluster_test.go:188–206 confirms file retains 'Personal:' when written via 'PERSONAL'"
    },
    {
      "concern": "correctness: read-time never validates — malformed values written directly to config file are emitted verbatim in config list --output json",
      "disposition": "addressed",
      "detail": "Load (config.go:249) unmarshals without calling ValidateA2ADefaultCluster; config_test.go:218–232 confirms hand-edited 'not a cluster!' is emitted verbatim in JSON output"
    }
  ]
}

ben-s-pull-request-reviewer

Step 4b-i: Candidates from glob

Changed files (from Step 0c): CHANGELOG.md README.md docs/a2a-default-cluster.md integration/cli_test.go pkg/cli/cli.go pkg/cli/config.go pkg/cli/config_test.go pkg/config/a2a_default_cluster.go pkg/config/a2a_default_cluster_test.go pkg/config/config.go specs/in-progress/063-a2a-default-cluster.md

Relevant triggers:

**/*.go + go.mod: matches .go files → go-context/cancel-check-in-loop, go-errors/*, go-factory/*, go-http-handler/*, go-testing/* rules
**/docs/*.md + **/docs/**: matches docs/a2a-default-cluster.md → ADR guide rules
agents/**/*.md, commands/**/*.md, skills/**/*: no match (no agent/command/skill files changed)

Step 4c-sel: Classify

For each candidate:

go-context/cancel-check-in-loop (trigger: **/*.go): skipped — the new a2a_default_cluster.go has no loops; the CLI config_test.go has DeferCleanup which is not a cancellable loop.
go-errors/inner-closure-no-double-wrap (trigger: **/*.go): skipped — no inner closures in the new code.
go-errors/no-bare-return-err (trigger: **/*.go, excludes *_test.go): skipped — new Go files contain no bare return err in non-test code; config_test.go is a test file (excluded by rule).
go-errors/no-context-background-in-business-logic (trigger: **/*.go): skipped — context.Background() appears only in test BeforeEach (line 234/515 in test files), not in business logic.
go-errors/no-fmt-errorf (trigger: **/*.go): skipped — ValidateA2ADefaultCluster uses errors.Errorf, not fmt.Errorf.
go-errors/sentinel-err-prefix-naming (trigger: **/*.go): skipped — no sentinel errors in the new code.
go-factory/main-holds-only-boot-lifecycle-config (trigger: **/*.go): skipped — no main.go changed.
go-factory/no-cleanup-return (trigger: **/*.go): skipped — no factory functions in new code.
go-factory/no-conditional-in-body (trigger: **/*.go): skipped — no factory functions in new code.
go-factory/no-error-return (trigger: **/*.go): skipped — no factory functions in new code.
go-factory/no-impl-in-factory-pkg (trigger: **/pkg/factory/**): skipped — pkg/config is not a factory package; a2aDefaultClusterWriter is a plain struct, not a factory.
go-http-handler/kebab-case-handler-files (trigger: **/*.go): skipped — no HTTP handler files changed.
go-http-handler/new-prefix-naming (trigger: **/*.go): skipped — no HTTP handlers changed.
go-http-handler/no-inline-background-handler (trigger: **/*.go): skipped — no inline background handlers.
go-http-handler/no-inline-error-handler (trigger: **/*.go): skipped — no inline error handlers.
go-http-service/admin-port-9090 (trigger: **/*.go): skipped — no HTTP service files changed.
go-http-service/canonical-admin-endpoints (trigger: **/*.go): skipped — no HTTP service files changed.
go-testing/counterfeiter-mocks-required (trigger: **/*.go): skipped — A2ADefaultClusterWriter is an interface with a concrete implementation (a2aDefaultClusterWriter); Counterfeiter generates mocks for interfaces, and the interface is already defined in a2a_default_cluster.go — no mock is needed since no test file creates a mock of it.
go-testing/libtime-injection-required (trigger: **/*_test.go): skipped — no time types used in new test code.
go-testing/main-test-with-compiles (trigger: **/*.go): skipped — no func main in new files.
go-testing/no-bare-error-call (trigger: **/*_test.go): skipped — test files use Gomega .To(BeNil()) pattern, not bare if err != nil.
go-testing/no-stdlib-table-tests (trigger: **/*_test.go): skipped — tests use Ginkgo Describe/Context/Entry tables, not stdlib table tests.
go-testing/no-testing-t-direct (trigger: **/*_test.go): skipped — no direct t.* calls in new test code.
go-testing/suite-test-file-required (trigger: **/*.go): skipped — config_test.go and a2a_default_cluster_test.go both have var _ = Describe(...) which is Ginkgo v2's suite initialization.
go-testing/suite-timeout-required (trigger: **/*_test.go): skipped — no ginkgo.It with timeout omitted in new code.
adr/required-for-irreversible-architecture-decisions (trigger: **/*.go, go.mod): skipped — no architectural change; a new config field + CLI verb + validator following existing patterns.
agent-cmd/* (triggers: agents/**/*.md, commands/**/*.md, skills/**/*): skipped — no agent/command/skill files created or changed.
go-context/cancel-check-in-loop (additional owner go-context-assistant): already assessed above.
go-errors/* (additional owner go-error-assistant): already assessed above.
go-factory/* (additional owner go-factory-pattern-assistant): already assessed above.
go-http-handler/* (additional owner go-http-handler-assistant): already assessed above.
go-testing/* (additional owner go-test-quality-assistant): already assessed above.

Selector clean — no applicable rules.

Step 4d-sel: Adjudicate

No applicable rules → no adjudication needed.

Step 5: Consolidated Report

Changed files: pkg/config/a2a_default_cluster.go, pkg/config/a2a_default_cluster_test.go, pkg/cli/config.go, pkg/cli/config_test.go, pkg/config/config.go, pkg/cli/cli.go, integration/cli_test.go, docs/a2a-default-cluster.md, README.md, CHANGELOG.md, specs/in-progress/063-a2a-default-cluster.md (11 files, 781 insertions)

Scope: Feature — new config key a2a_default_cluster with RFC 1123 validation, atomic writer, CLI verb, and JSON emission.

Must Fix (Critical)

None.

Should Fix (Important)

None.

Nice to Have (Optional)

None.

Selector Mode Traceability

Candidates: 26 judgment rules from glob
Applicable: 0
Skipped: all 26 — see Step 4c-sel above for individual justifications

Notes

The pkg/config/a2a_default_cluster.go follows the same pattern as baseline.go (same findVaultKey, writeFileAtomic, yaml.Marshal reuse). The pkg/cli/config.go follows the same pattern as createConfigSetBaselineCommand. No deviations from established patterns.

The specs/in-progress/063-a2a-default-cluster.md spec is in specs/in-progress/ (not a live spec) — this is the dark-factory spec file that drove the implementation. Per the plan's scope it is correctly included in the diff.

The integration/cli_test.go adds only a table entry for the new verb, which is the minimal integration coverage appropriate for a CLI verb.

Mechanical funnel: ⚠️ COULD NOT RUN — ast-grep runner failed with "Argument list too long" (/home/claude/.claude/plugins/marketplaces/coding/scripts/ast-grep-runner.sh: line 246: /usr/bin/jq: Argument list too long). This is a toolchain failure; the runner cannot process this diff. No MUST-tier mechanical findings were produced. The diff is a config-key addition (YAML tags + validation + writer), not a code pattern that would typically trigger YAML-based AST rules — no blocking defects are expected from the missing pass.

Verdict

{
  "verdict": "approve",
  "summary": "Feature adds a2a_default_cluster config key with RFC 1123 validation, atomic writer, CLI verb set-a2a-default-cluster, and JSON emission — cleanly following the existing baseline config pattern. Comprehensive test coverage includes unit tests for validation, writer isolation, atomic writes, vault matching, and CLI integration. No violations found against judgment rules (mechanical funnel unavailable due to toolchain failure; diff is config-key addition, not code-pattern relevant to YAML AST rules).",
  "comments": [],
  "concerns_addressed": [
    {
      "concern": "security: RFC 1123 regex validates cluster name on write; colon rejected to prevent target-string impersonation",
      "disposition": "addressed",
      "detail": "ValidateA2ADefaultCluster uses regexp ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ and explicitly rejects colons before any write; tested in a2a_default_cluster_test.go and config_test.go"
    },
    {
      "concern": "correctness: ValidateA2ADefaultCluster called before any file write; failures leave config byte-identical",
      "disposition": "addressed",
      "detail": "ValidateA2ADefaultCluster is called first in SetA2ADefaultCluster; writeFileAtomic is only called after all validations pass; tested in a2a_default_cluster_test.go 'rejects a malformed cluster name and leaves the file byte-identical'"
    },
    {
      "concern": "correctness: A2ADefaultCluster field uses omitempty on both yaml and json tags",
      "disposition": "addressed",
      "detail": "Verified in pkg/config/config.go line 64: `yaml:\"a2a_default_cluster,omitempty\" json:\"a2a_default_cluster,omitempty\"`"
    },
    {
      "concern": "correctness: Read-time never validates; hand-edited malformed values are emitted verbatim",
      "disposition": "addressed",
      "detail": "ValidateA2ADefaultCluster is only called in the writer; the loader never validates; tested in config_test.go 'emits a hand-edited malformed value verbatim rather than refusing it'"
    },
    {
      "concern": "correctness: yaml.Marshal roundtrip discards comments and unmodeled keys",
      "disposition": "not-an-issue",
      "detail": "Documented in docs/a2a-default-cluster.md 'A successful write re-serializes the whole file from the config struct. Other keys the config struct models survive; comments and any key the struct does not model do not.' This is accepted existing behavior (same as baseline verb)"
    },
    {
      "concern": "correctness: findVaultKey matches vault name case-insensitively but preserves file's original key spelling",
      "disposition": "addressed",
      "detail": "findVaultKey uses strings.EqualFold for matching; the matchedKey is used to index into cfg.Vaults, preserving original spelling; tested in a2a_default_cluster_test.go 'with a mixed-case vault key'"
    },
    {
      "concern": "tests: Comprehensive test suite covering valid/invalid RFC 1123 shapes, atomic write, vault isolation, case-insensitive matching, malformed config rejection",
      "disposition": "addressed",
      "detail": "a2a_default_cluster_test.go has 241 lines covering ValidateA2ADefaultCluster and A2ADefaultClusterWriter; config_test.go has 167 lines for CLI integration"
    }
  ]
}

ben-s-pull-request-reviewer

Now I have the full diff and all key source files. Let me review the concerns from the plan systematically.

Reviewing all plan concerns:

ValidateA2ADefaultCluster regex + length checks — Regex ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ (line 20 of a2a_default_cluster.go) + length check at line 36. This is exhaustive for the stated shapes (lowercase, no leading/trailing hyphen, max 63 chars). The test suite covers exactly 63 chars at line 527 and 64 chars rejection at line 549.
findVaultKey case-insensitive match — findVaultKey (baseline.go:119) returns the matched key (preserving original spelling), not a constructed value. No path construction.
writeFileAtomic secure permissions — os.CreateTemp uses Go default 0600. Temp removed on all error paths. Verified at baseline.go:137-161.
yaml.Marshal round-trip — All Vault struct keys are modelled. omitempty on a2a_default_cluster (config.go:64) means absent values don't re-inject empty keys. Tests verify preservation of other keys (a2a_default_cluster_test.go:123-136).
Both omitempty tags — yaml:"a2a_default_cluster,omitempty" json:"a2a_default_cluster,omitempty" at config.go:64. Both tags present as required.
findVaultKey returns matched key spelling — findVaultKey returns matchedKey (the key from range iteration, preserving file's spelling). The mixed-case test at a2a_default_cluster_test.go:188-205 confirms Personal: is preserved after writing.
writeFileAtomic cleanup on error — All error paths (lines 141, 147, 152, 157 in baseline.go) call os.Remove(tmp.Name()). Temp file has 0600 from os.CreateTemp.
Test coverage — Both test files have comprehensive coverage including the 63/64 char boundary, case-insensitive vault matching, malformed value round-trip, and file byte-identical on refusal.
Integration tests — integration/cli_test.go:138 adds the new verb to the integration table. pkg/cli/config_test.go covers JSON emission, omitempty for unset vault, hand-edited malformed value, target-string refusal, absent vault, and other-vault isolation.

Overall assessment: The feature is well-implemented with comprehensive tests. No issues found.

{
  "verdict": "approve",
  "summary": "The `a2a_default_cluster` feature adds a validated, atomic, omitempty config key with full test coverage. All plan concerns were verified against the source: RFC 1123 validation is exhaustive, vault key matching preserves original spelling, atomic writes use 0600 permissions, round-trip preserves all struct-modelled keys, and integration tests cover all acceptance criteria.",
  "comments": [],
  "concerns_addressed": [
    {
      "concern": "security: ValidateA2ADefaultCluster accepts untrusted argv input; regex and length checks exhaustive",
      "disposition": "not-an-issue",
      "detail": "Regex ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ + len > 63 check covers all stated shapes (lowercase, no underscore/space/dot/slash/non-ASCII, no leading/trailing hyphen). Confirmed in a2a_default_cluster.go:20,36 and a2a_default_cluster_test.go:518-552"
    },
    {
      "concern": "security: findVaultKey does not allow path traversal or key injection",
      "disposition": "not-an-issue",
      "detail": "findVaultKey (baseline.go:119) returns the matched map key directly; no path construction or string concatenation with user input. Used at a2a_default_cluster.go:98"
    },
    {
      "concern": "security: writeFileAtomic creates temp file with secure permissions and cleans up on error",
      "disposition": "not-an-issue",
      "detail": "os.CreateTemp uses Go default 0600. All error paths (lines 141,147,152,157 of baseline.go) remove the temp file. Confirmed at baseline.go:137-161"
    },
    {
      "concern": "correctness: yaml.Marshal round-trip preserves all modelled keys",
      "disposition": "not-an-issue",
      "detail": "Vault struct models all config keys. A2ADefaultCluster has omitempty so absent values don't re-inject. Test at a2a_default_cluster_test.go:123-136 verifies other keys survive"
    },
    {
      "concern": "correctness: Both yaml and json omitempty tags present; JSON omits empty key",
      "disposition": "not-an-issue",
      "detail": "config.go:64 has yaml:a2a_default_cluster,omitempty json:a2a_default_cluster,omitempty. Test at config_test.go:207-215 verifies key absent from JSON when unset"
    },
    {
      "concern": "correctness: findVaultKey returns matched key spelling, not canonical lowercase",
      "disposition": "not-an-issue",
      "detail": "findVaultKey returns the map key from range iteration, preserving file's original spelling. Test at a2a_default_cluster_test.go:188-205 verifies Personal: is preserved after writing with PERSONAL"
    },
    {
      "concern": "correctness: atomic write temp file cleaned up on crash between write and rename",
      "disposition": "not-an-issue",
      "detail": "writeFileAtomic (baseline.go:137) creates temp in same dir; rename is atomic; all error paths remove temp. Docs at docs/a2a-default-cluster.md:120-122 state the contract"
    },
    {
      "concern": "tests: comprehensive ValidateA2ADefaultCluster coverage including 63/64 char boundary",
      "disposition": "not-an-issue",
      "detail": "Test at a2a_default_cluster_test.go:527 accepts strings.Repeat('a',63); test at line 549 rejects strings.Repeat('a',64)"
    },
    {
      "concern": "tests: integration tests cover all set-a2a-default-cluster scenarios",
      "disposition": "not-an-issue",
      "detail": "integration/cli_test.go:138 adds the verb to the integration table. pkg/cli/config_test.go:172-277 covers: success, raw-file key, JSON emission, omitempty for unset, malformed verbatim, target-string refusal, absent vault, other-vault isolation, argument count"
    }
  ]
}

chore(spec): a2a-default-cluster

c61c822

bborbe mentioned this pull request Oct 20, 2026

feat: a vault names its own default A2A cluster bborbe/claude-supervisor#460

Merged

bborbe marked this pull request as ready for review October 20, 2026 18:03

Merge remote-tracking branch 'origin/master' into feat/a2a-default-cl…

e6a3a8f

…uster # Conflicts: # CHANGELOG.md

ben-s-pull-request-reviewer Bot requested changes Oct 20, 2026

View reviewed changes

bborbe merged commit f3273ec into master Oct 20, 2026
3 checks passed

bborbe deleted the feat/a2a-default-cluster branch October 20, 2026 18:56

Navigation Menu

Repository navigation

df: a2a-default-cluster - #325

df: a2a-default-cluster#325
bborbe merged 3 commits into
masterfrom
feat/a2a-default-cluster

bborbe commented Oct 20, 2026

Uh oh!

ben-s-pull-request-reviewer Bot left a comment

Uh oh!

ben-s-pull-request-reviewer Bot left a comment

Uh oh!

ben-s-pull-request-reviewer Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

bborbe commented Oct 20, 2026

Uh oh!

ben-s-pull-request-reviewer Bot left a comment

Choose a reason for hiding this comment

PR Review: df: a2a-default-cluster (PR #325 → feat/a2a-default-cluster)

Scope

Must Fix (Critical) — none found

Should Fix (Important) — none found

Nice to Have (Optional) — none found

Manual Review

Selector Mode Traceability

Uh oh!

ben-s-pull-request-reviewer Bot left a comment

Choose a reason for hiding this comment

Step 4b-i: Candidates from glob

Step 4c-sel: Classify

Step 4d-sel: Adjudicate

Step 5: Consolidated Report

Must Fix (Critical)

Should Fix (Important)

Nice to Have (Optional)

Selector Mode Traceability

Notes

Verdict

Uh oh!

ben-s-pull-request-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

PR Review: `df: a2a-default-cluster` (PR #325 → `feat/a2a-default-cluster`)