fix: add diagnostic warnings for config loading failures

[cabana8471-arch]

Summary

• Added logging.warning() calls when config files fail to load
• Users can now diagnose why their settings aren't being applied

Problem

When users configure .autocoder/allowed_commands.yaml or ~/.autocoder/config.yaml, they get no feedback when config fails to load. The functions returned None silently on ANY error:

Error Type	Previous Behavior
YAML syntax error	Returns None, no warning
Missing version field	Returns None, no warning
Invalid structure	Returns None, no warning
File permissions	Returns None, no warning
Empty file	Returns None, no warning
Over 100 commands	Returns None, no warning

Users couldn't distinguish between "file not found" (expected) and "file has errors" (unexpected).

Fix

Added logger.warning() calls for each validation failure in:

• load_org_config() - org-level config loading
• load_project_commands() - project-level config loading

Also added .resolve() to project path handling for symlink support.

Example Output

WARNING: Project config at /path/.autocoder/allowed_commands.yaml missing required 'version' field
WARNING: Failed to parse org config at ~/.autocoder/config.yaml: mapping values are not allowed here

Testing

# Test with invalid YAML: .autocoder/allowed_commands.yaml
version: 1
commands:
  - name: test
  - invalid yaml here

1. Start agent
2. Should see warning in logs with file path and error details

Fixes #91

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Configuration validation now emits detailed, contextual warnings for invalid or missing entries instead of failing silently.
  • YAML parsing and file I/O errors produce clear warnings with context.
  • Project configuration path resolution improved.
  • Stricter validation and enforced limits for command lists to prevent malformed configs.

• Tests

  • Added a test to reject empty command names in project configuration.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds module-level logging and extensive warning-based validation to org and project config loaders in security.py, replacing silent failures with explicit warnings and using Path.resolve() for project config path resolution.

Changes

Cohort / File(s)	Summary
Config validation & logging security.py	Adds logging import and module-level logger. Enhances _load_org_config() and _load_project_config() with warnings for empty/invalid YAML, non-dict root, missing version, invalid allowed_commands/commands types, per-entry type/name validation (with index context), list limits (100 commands), YAML/I/O exception logging, and pkill_processes normalization/validation. project_dir.resolve() used for project config path. Validation failures return None.
Project config tests test_security.py	Adds test case in test_project_commands() to verify that empty command names in project YAML cause loading failure (None), ensuring validation of command entries works as expected.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰
Logs now sing where silence lay,
Warnings hop to light the way.
Names and lists checked, each line in sight,
Paths resolved, no mystery tonight.
🥕

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: add diagnostic warnings for config loading failures' accurately summarizes the main change—adding logger.warning calls for configuration validation and parsing failures.
Linked Issues check	✅ Passed	The PR successfully addresses issue #91's objectives by adding logger.warning calls for config loading failures, enabling users to distinguish between file absence and validation errors, and implementing empty command name rejection.
Out of Scope Changes check	✅ Passed	All changes are within scope: logger.warning calls for validation/parsing failures in load_org_config and load_project_commands, .resolve() for symlink support, and a test for empty command names align directly with issue #91's objectives.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[cabana8471-arch]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[leonvanzyl]

PR Review Summary

Verdict: APPROVE (after resolving conflicts)

Risk Level: LOW

---

Security Analysis

• No concerns - all validation logic preserved
• Only file paths and error types are logged (no secrets exposed)
• The .resolve() change is safe (already sandboxed upstream)
• Minor security improvement: empty command names now rejected in project configs

Code Quality

• Logging level (WARNING) is appropriate and matches codebase patterns
• Log messages are clear and actionable
• Exception handling split improves diagnostics

Test Coverage

• All existing tests pass
• The behavioral change (empty string validation) aligns with load_org_config()

---

Action Required

Please resolve the merge conflicts with master before this can be merged.

---

Optional Improvements

1. Consider adding a test for empty command name validation in load_project_commands():

def test_empty_command_name_rejected(self):
    config_path = self.project_dir / ".autocoder" / "allowed_commands.yaml"
    config_path.write_text('version: 1\ncommands:\n  - name: ""\n')
    result = load_project_commands(self.project_dir)
    self.assertIsNone(result)

2. Update PR description test count (136 → 162 unit tests)

---

Overall this is a quality improvement that adds valuable diagnostic information for users debugging config loading issues.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

security.py (2)

492-506: Missing diagnostic warnings for pkill_processes validation failures.

The pkill_processes validation returns None silently on failures (non-list type, non-string entries, invalid process names), which is inconsistent with other validation in this function that logs warnings. Per the PR objective of providing visibility into config loading failures, these should also log warnings.

Proposed fix to add missing warnings

         # Validate pkill_processes if present
         if "pkill_processes" in config:
             processes = config["pkill_processes"]
             if not isinstance(processes, list):
+                logger.warning(f"Org config at {config_path}: 'pkill_processes' must be a list")
                 return None
             # Normalize and validate each process name against safe pattern
             normalized = []
-            for proc in processes:
+            for i, proc in enumerate(processes):
                 if not isinstance(proc, str):
+                    logger.warning(f"Org config at {config_path}: pkill_processes[{i}] must be a string")
                     return None
                 proc = proc.strip()
                 # Block empty strings and regex metacharacters
                 if not proc or not VALID_PROCESS_NAME_PATTERN.fullmatch(proc):
+                    logger.warning(f"Org config at {config_path}: pkill_processes[{i}] has invalid value '{proc}'")
                     return None
                 normalized.append(proc)
             config["pkill_processes"] = normalized

---

574-588: Missing diagnostic warnings for pkill_processes validation in project config.

Same issue as in load_org_config(): the pkill_processes validation returns None silently without logging warnings, inconsistent with other validation in this function.

Proposed fix to add missing warnings

         # Validate pkill_processes if present
         if "pkill_processes" in config:
             processes = config["pkill_processes"]
             if not isinstance(processes, list):
+                logger.warning(f"Project config at {config_path}: 'pkill_processes' must be a list")
                 return None
             # Normalize and validate each process name against safe pattern
             normalized = []
-            for proc in processes:
+            for i, proc in enumerate(processes):
                 if not isinstance(proc, str):
+                    logger.warning(f"Project config at {config_path}: pkill_processes[{i}] must be a string")
                     return None
                 proc = proc.strip()
                 # Block empty strings and regex metacharacters
                 if not proc or not VALID_PROCESS_NAME_PATTERN.fullmatch(proc):
+                    logger.warning(f"Project config at {config_path}: pkill_processes[{i}] has invalid value '{proc}'")
                     return None
                 normalized.append(proc)
             config["pkill_processes"] = normalized

[cabana8471-arch]

Added the suggested test for empty command name validation in project config. Test count updated to 163. Ready for merge! 🚀