Follow-up: bin/agent-coord duplicates three policy/validation code paths that must stay in sync

[justin808]

Surfaced by review of #105 (head 70bc6b6). Three independent reviewer findings describe the same shape: bin/agent-coord hand-rolls the same logic in more than one place, so a future fix has to be applied twice or thrice to stay correct. None of these are defects today — they are divergence risks in a file whose whole purpose is a security boundary, which is exactly where silent drift is most expensive.

Deliberately excluded from #105: that PR is already a ~2400-line security-sensitive change tripping the autonomous-merge changed-lines-limit, and pure refactors of pre-existing code would widen the blast radius of a diff that needs careful human review. Filed here so the cleanup is not lost.

1. validate_resolved_directory_chain! duplicates validate_secure_directory_chain!s component walk

#105 (comment)

Both split an expanded path into components, walk from /, and call validate_secure_directory_component! per component with the same leaf computation. The only real difference is that validate_secure_directory_chain! also handles symlink resolution/creation and calls into validate_resolved_directory_chain! after resolving. Since the latter is only ever invoked from inside the former, the walk-and-check loop could be one shared helper parameterized by "handle symlinks or not", instead of two near-identical ~15-line copies. A future change to how leaf is computed would need applying twice — precisely the TOCTOU-prone divergence this code is hardened against elsewhere.

2. validate_config_api_url! duplicates HttpStore#parse_base_urls URL policy

#105 (comment)

Both implement: scheme must be http/https with a non-empty host; no query or fragment; plain http rejected unless the hostname is in LOOPBACK_HOSTS. validate_config_api_url! even reaches into HttpStore::LOOPBACK_HOSTS to share the constant, but reimplements the surrounding logic and uses a different error type (Error vs OperationalError). If the loopback/https policy changes in one place — say a new trusted loopback alias — nothing signals that the other needs the same update, so agent-coord config set --api-url could accept a URL the HTTP backend later rejects at request time, or vice versa.

Note this already bit once: #105 needed a separate "Fix IPv6 loopback URL validation" commit (25121dc8) touching both sides.

3. Three reimplementations of the same cli > process_env > user_config precedence chain

#105 (comment)

effective_config_value, the effective_backend_env_value / process_selects_backend? / backend_env_source group, and effective_policy each resolve the same tiered precedence differently. effective_config_value treats an empty string as unset via .to_s.empty?; effective_backend_env_value returns raw values including empty strings, forcing callers like resolve_backend_env to separately normalize (env_api = nil if env_api == ""); effective_policy folds a fourth policy_file tier inline rather than composing. A precedence or empty-string change made in one is not guaranteed to be mirrored in the other two.

The empty-string inconsistency here is not hypothetical — it is the root of the --state-root "" and config show --backend "" reports also raised against #105.

Proposed fix

Factor each cluster into one shared implementation. Best done as a standalone refactor PR with no behavior change, so the diff is reviewable as pure restructuring and the existing suite acts as the regression net.

[justin808]

A fourth site in the same family, from the review wave on head b664d34: #105 (comment)

4. Two independent "where is the user config" resolvers

selected_user_config_path derives the AGENT_COORD_ENV_FILE → XDG_CONFIG_HOME → $HOME/.config → agent-coord/env precedence chain from scratch. The pre-existing consumer_env_files computes essentially the same three-branch precedence for the legacy split-brain detector — but as a lenient regex scan over raw file lines, rather than through the strict ownership/permission/symlink-validated loader. consumer_split_brain_env_file never reuses @user_config_path.

So the same question ("which file is the user config?") is answered by two mechanisms with different strictness. They can disagree, and the lenient one feeds a diagnostic that is supposed to describe the strict one.

Also worth recording against item 2: the reviewer independently noted that this PR had to add the query/fragment check to validate_config_api_url! and HttpStore#parse_base_url in parallel — a second instance of the near-drift already evidenced by the IPv6 commit 25121dc8. That duplication has now been reported three times by two different reviewers across three separate review waves, which is a reasonable signal it is worth doing.

[justin808]

A third copy of the loopback check, found while fixing the case-sensitivity issue on head e96b119.

local_http_url? (bin/agent-coord:2956, doctor diagnostics) performs its own loopback test using uri.host rather than uri.hostname. Two consequences relative to the two validators:

• uri.host keeps IPv6 brackets, so it compares [::1] against a ::1 allowlist.
• It remains case-sensitive, so LOCALHOST is not recognised as loopback there even though both validators now accept it.

It is diagnostic-only — it decides whether to print a --local hint — so it is not a correctness defect today, and it was left alone as out of scope for that fix. But it means the loopback rule now exists in three places with three slightly different behaviours, which strengthens the case for the shared validator this issue proposes.

Two further frictions observed while keeping the existing two copies in sync, worth knowing before the refactor:

• They raise different exception classes (Error vs OperationalError) with differently-phrased messages, so a shared constant (VALID_PORT_RANGE) could be extracted but the check itself could not.
• The rubocop line-length limit forced the two port checks into different syntactic shapes (modifier vs block form), so they read as less parallel than they actually are — which is exactly the kind of cosmetic divergence that hides semantic divergence later.

[justin808]

Three more sites in this family, from the review wave on head e96b119. That brings the cluster to seven.

5. Three structurally identical warn_cli_* helpers, already drifted

#105 (comment)

warn_cli_state_root_override / warn_cli_api_url_override / warn_cli_backend_override are copy-paste differing only in a literal message string and a configured_selector? key — and they have already diverged: only warn_cli_backend_override carries the empty-value guard. Together with resolve_cli_backend? and resolve_configured_backend, the conflict-warning system is ~10 hardcoded pairwise if/elsif branches rather than one "rank selectors by precedence, warn about winner plus next-highest displaced loser" routine. Adding one more backend selector means touching at least five methods.

Noting for the record that these three helpers were added by #105 itself, so this is self-inflicted duplication rather than pre-existing — which makes it a good candidate to fold into the same refactor.

6. A third copy of the temp-file + O_EXCL + fsync + rename + parent-fsync skeleton

#105 (comment)

write_canonical_config_file / write_private_config_temp_file restate a sequence that already exists in LocalStore#atomic_write_json and Runner#log_sync_replace. All three share only the trailing AgentCoord.fsync_parent_directory helper. This is a security-sensitive durability invariant that now has to be re-verified in three places; an fsync-ordering fix in one will not propagate.

7. Redundant directory-chain walks within a single config load

#105 (comment)

with_optional_config_read_lock plus the read_private_config_file / validate_private_config_file! calls it wraps perform the full ownership/permission chain walk 2-3 times for the same directory in one load — once resolving the lock directory via File.realpath, once validating the lock file, once validating the config file, and a fourth time if a legacy policy file is read. The chain cannot change between those calls within one invocation.

This one is partly an efficiency argument rather than a pure correctness one, but it shares the root cause: the same walk implemented and invoked in several places with no shared memoization or single entry point. Note it compounds with the unconditional load_user_configuration! on every CLI invocation, which is a separate open question on #105.