fix(cli): Honor excluded_attributes from config file

[kami619]

Description

fix(cli): Honor excluded_attributes from config file

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Refactoring (no functional changes)
• Performance improvement
• Test coverage improvement

Related Issues

Fixes #302

Changes Made

The excluded_attributes option in configuration files was being ignored. Only the --exclude CLI flag worked. This fix merges exclusions from both sources (CLI and config) before filtering assessors, and adds validation so invalid attribute IDs in config files now produce clear error messages.

• Merge CLI --exclude and config.excluded_attributes into a single set
• Validate all exclusions against valid attribute IDs
• Add 4 regression tests for issue [BUG] excluded_attributes in config file is not being honored #302

Testing

• Unit tests pass (pytest)
• Integration tests pass
• Manual testing performed
• No new warnings or errors

Checklist

• My code follows the project's code style
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published

[github-actions]

AgentReady Code Review — PR #306

fix(cli): Honor excluded_attributes from config file
Reviewed against: AgentReady v2.28.1 self-assessment (80.0/100 Gold)

---

Summary

This PR correctly fixes issue #302 by merging excluded_attributes from config files with CLI --exclude arguments before filtering assessors. The fix is sound, the logic is clean, and regression tests cover the key scenarios. One semantic concern and one test gap are noted below.

---

AgentReady Attribute Impact

Attribute	Tier	Current Score	PR Impact
test_coverage	2	✅ Pass	↑ +130 lines of test code added
cyclomatic_complexity	3	✅ 3.1 avg	Neutral (2 new branches, low complexity)
type_annotations	1	✅ 96.5%	Neutral (no new function signatures)
conventional_commits	2	❌ Not configured	PR uses fix(cli): prefix — correct format

No regressions to assessed attributes.

---

Code Analysis

✅ Correctness — src/agentready/cli/main.py

The merge logic is clean and handles all edge cases correctly:

all_exclusions = set(exclude or [])
if config and config.excluded_attributes:
    all_exclusions.update(config.excluded_attributes)

• exclude=None (default): set(None or []) → set() ✓
• Config excluded_attributes = [] (empty list): falsy, skipped ✓
• Both sources populated: correctly unioned into one set ✓

The verbose echo condition change from if verbose and exclude: to if verbose: (inside if all_exclusions:) is also correct — the old inner and exclude guard was redundant inside if exclude:, and now config-only exclusions are correctly echoed in verbose mode.

⚠️ Semantic Issue — Exception Type for Config Errors

raise click.BadParameter(
    f"Invalid attribute ID(s): {', '.join(sorted(invalid_ids))}. "
    f"Valid IDs: {', '.join(sorted(valid_ids))}"
)

click.BadParameter is designed for CLI parameter validation failures. When the invalid ID originates from the config file rather than --exclude, the error renders as:

Error: Invalid value: Invalid attribute ID(s): nonexistent_attribute...

This attributes the error to a CLI parameter when it actually came from the config file, which can mislead users. Consider click.ClickException for config-sourced errors, or include source context in the message to distinguish where the invalid ID came from.

Minor UX issue — non-blocking.

✅ Test Quality — tests/unit/cli/test_main.py

Three regression tests cover the required scenarios:

Test	Scenario	Status
test_run_assessment_excludes_attributes_from_config_issue_302	Config-only exclusion	✓
test_run_assessment_merges_cli_and_config_exclusions_issue_302	CLI + Config merged	✓
test_run_assessment_validates_config_excluded_attributes_issue_302	Invalid ID in config rejected	✓

Test gap: The verbose logging condition was modified (removed the redundant and exclude guard), but no test validates that verbose=True echoes exclusions when they come from config alone. Low risk since the logic is simple, but the behavioral change is untested.

---

Security

No security concerns. Validation against valid_ids protects against arbitrary attribute IDs from config files. No shell execution, SQL, or untrusted data paths are involved in the changed code.

---

Verdict

Approve with minor notes. The bug fix is correct, well-tested (5:1 test-to-production-code ratio), and consistent with existing patterns in the codebase. Two non-blocking items:

1. click.BadParameter is semantically wrong for config file validation errors — click.ClickException would be more accurate
2. Verbose logging behavior change is untested (low risk)

---

Review generated by AgentReady code review skill • Self-assessment: 80.0/100 Gold

[github-actions]

AgentReady Code Review: PR #306 — fix(cli): Honor excluded_attributes from config file

Reviewer: AgentReady automated review
Verdict: Approve with minor notes — clean, focused fix

---

Summary

This PR fixes a real bug (#302): excluded_attributes in config files was silently ignored while only --exclude CLI flags worked. The fix is minimal and correct: merge both exclusion sources into a single set before validation and filtering.

---

Code Quality

src/agentready/cli/main.py

The logic change is clean and idiomatic:

all_exclusions = set(exclude or [])
if config and config.excluded_attributes:
    all_exclusions.update(config.excluded_attributes)

set(exclude or []) handles the None case without a separate if check. The set union provides deduplication for free if CLI and config specify the same attribute.

The verbose output change from if verbose and exclude: to if verbose: is safe — it is still inside if all_exclusions:, so the echo only fires when there are exclusions. Removing the redundant inner guard is correct.

Minor: click.BadParameter is semantically off for config-sourced errors.
click.BadParameter implies the invalid value came from a CLI parameter. When the bad ID came from a config file, a click.UsageError with a message indicating the config file path would be more accurate and less confusing in tracebacks. Not a blocker, but worth addressing.

Minor: Verbose output doesn't distinguish source.
The merged message "Excluded N attribute(s): ..." doesn't tell the user whether exclusions came from --exclude or excluded_attributes in config. If both are active, users have no way to trace why a given attribute was excluded without re-reading both sources. Consider a follow-up showing the source breakdown.

Note: Config.is_excluded() exists but isn't used here.
models/config.py:133 defines is_excluded(attribute_id). The current approach (merging into a set at the filtering stage) is actually cleaner for the CLI context and avoids an N×M loop, so this isn't a regression.

---

Test Coverage

Three regression tests added, all with clear docstrings explaining the issue context.

Scenario	Covered
Config-only exclusions applied	Yes
CLI + config exclusions merged	Yes
Invalid config attribute ID rejected	Yes
Duplicate exclusion in CLI + config	Implicit (set semantics cover it, no explicit test)
verbose=True with config exclusions	Not tested

Minor: call_args[0][0] is fragile.

assessors = call_args[0][0]  # First positional argument

If scanner.scan() is ever called with keyword arguments, this index breaks silently. Prefer call_args.args[0] (Python 3.8+ unittest.mock API) for forward compatibility.

---

AgentReady Attribute Compliance

Attribute	Impact
test_coverage	Positive — 121 lines of tests added for 18 lines of logic
bug_fix_quality	Positive — fix is targeted, minimal, no collateral changes
code_comments	Positive — # Fix for #302: comment clearly links code to issue
type_annotations	Neutral — no new signatures introduced

---

Security

No security concerns. The validation path (valid_ids - all_exclusions) prevents injection of non-existent attribute IDs from either source. No subprocess calls, no user-controlled paths passed unsanitized.

---

Verdict

The fix is correct, well-scoped, and meaningfully tested. The two minor issues (error type and fragile test indexing) are non-blocking. Recommend merging after the draft is finalized and integration tests are confirmed passing.

[github-actions]

📈 Test Coverage Report

Branch	Coverage
This PR	66.1%
Main	66.0%
Diff	✅ +0.1%

---

Coverage calculated from unit tests only

[github-actions]

Code Review - Follow-up to Automated Reviews

This review follows the two automated comments posted at 22:17Z and 22:19Z. The latest commit (pushed at 22:29Z) addressed the concerns raised in those earlier reviews, and the overall direction here is solid. The core bug fix is correct: config.excluded_attributes was silently ignored before this PR, and the merge logic using set union (cli_exclusions | config_exclusions) is the right approach.

---

What Was Addressed in the Latest Commit

• Differentiated error types: CLI invalid IDs now raise click.BadParameter while config file invalid IDs raise click.ClickException with source context. This is a meaningful improvement over treating both sources identically.
• 4th test added: A verbose output test (test_run_assessment_verbose_echoes_config_exclusions_issue_302) was added, covering the echo path that previously had no test.

Good follow-through on the review feedback.

---

Remaining Issues

1. Fragile mock call introspection (call_args[0][0])

In two of the new tests, the assessor list is extracted like this:

call_args = mock_scanner.scan.call_args
assessors = call_args[0][0]  # First positional argument

The [0][0] index tuple access is a legacy unittest.mock pattern that is fragile - it silently returns None if the call signature changes (e.g., if scan() is ever called with keyword arguments instead of positional ones). The more robust and readable modern API is:

assessors = call_args.args[0]

This uses the named attribute on call_args rather than relying on positional tuple indexing, and it will raise a clear AttributeError rather than silently passing with wrong data if the call signature shifts. Both tests that do mock introspection (test_run_assessment_excludes_attributes_from_config_issue_302 and test_run_assessment_merges_cli_and_config_exclusions_issue_302) should use .args[0].

2. PR description says "Add 3 regression tests" but 4 were added

The description still reads "Add 3 regression tests" but the final commit includes four:

1. test_run_assessment_excludes_attributes_from_config_issue_302
2. test_run_assessment_merges_cli_and_config_exclusions_issue_302
3. test_run_assessment_validates_config_excluded_attributes_issue_302
4. test_run_assessment_verbose_echoes_config_exclusions_issue_302

Minor, but worth updating the PR description so the history is accurate.

3. Silent drop of config errors when CLI also has invalid IDs

The validation logic raises errors in sequence:

if invalid_from_cli:
    raise click.BadParameter(...)
else:
    raise click.ClickException(...)

When both invalid_from_cli and invalid_from_config are non-empty, only the CLI error is raised and the config error is silently dropped. A user with invalid IDs in both sources will fix the CLI error, re-run, and then discover the config error - two round trips instead of one. Consider reporting both in a single error message, or at minimum noting the config errors as warnings alongside the CLI BadParameter:

if invalid_from_cli:
    msg = f"Invalid attribute ID(s): {', '.join(sorted(invalid_from_cli))}."
    if invalid_from_config:
        msg += f" Also invalid in config file: {', '.join(sorted(invalid_from_config))}."
    msg += f" Valid IDs: {', '.join(sorted(valid_ids))}"
    raise click.BadParameter(msg)

This is a minor UX issue, not a correctness bug.

4. capsys vs CliRunner for the verbose test

The verbose test captures click.echo() output using pytest's capsys:

captured = capsys.readouterr()
assert "Excluded 1 attribute(s)" in captured.out

capsys captures at the file-descriptor level and can be unreliable with Click's output, which routes through its own stream abstraction. The established pattern in the AgentReady test suite for asserting CLI output is to use Click's CliRunner, which captures output through Click's own machinery and is always reliable regardless of how Click's internal streams are configured:

from click.testing import CliRunner

runner = CliRunner()
result = runner.invoke(assess, ["--verbose", str(test_repo), "--config", str(config_file)])
assert "Excluded 1 attribute(s)" in result.output

If the existing capsys approach happens to pass in CI it is not wrong, but it is more brittle than necessary and inconsistent with the Click testing idiom. This is the most actionable of the remaining issues to fix before merging.

---

Overall Verdict

The fix is correct and the test coverage is meaningfully better than before this PR. Issues 2 and 3 above are minor polish items. Issues 1 and 4 are worth addressing before merge for robustness, but neither represents a risk to correctness of the feature itself. Approving with these notes - happy to re-review after the small cleanups.

[github-actions]

Outdated review (click to expand)

🤖 AgentReady Code Review

PR: #306 fix(cli): Honor excluded_attributes from config file
Commit: 5cece06
PR Status: 2 issues found (0 🔴 Critical, 1 🟡 Major, 1 🔵 Minor)
Score Impact: Current 80.0/100 → 80.1/100 if all issues fixed (+0.1 from test gap closure)
Certification: Gold → Gold (no change)

Note: This PR is in Draft status. Review posted at author's request.

---

What this PR does well

The core fix is correct: excluded_attributes from config was silently ignored because validation and filtering only checked the --exclude CLI flag. The merge using set union (cli_exclusions | config_exclusions) is idiomatic and handles all degenerate cases (None, empty list, duplicates across sources) without explicit guards. Previous review feedback has been fully addressed:

• ✅ call_args.args[0] replaces fragile call_args[0][0] tuple indexing in tests
• ✅ CliRunner replaces capsys for reliable Click output capture
• ✅ Config-only errors use click.ClickException (not BadParameter) with source attribution
• ✅ Mixed-source errors produce a combined message so the user can fix both in one round trip
• ✅ E2E test corrected from repomix_config (unregistered assessor, never in findings) to openapi_specs (registered Tier 3 assessor, fails when no spec file present)

---

🟡 Major Issues (Confidence 80-89) - Manual Review Required

1. Combined-error code path is untested

Attribute: test_coverage (Tier 2) — CLAUDE.md § Test Requirements
Confidence: 83%
Score Impact: −0.1 points (test_coverage gap in new branch)
Location: src/agentready/cli/main.py — mixed-source validation block (SHA 5cece06)

Issue Details:

Four tests cover these paths:

Test	Path exercised
test_...excludes_attributes_from_config	Config-only exclusions applied
test_...merges_cli_and_config_exclusions	Both sources, all valid IDs
test_...validates_config_excluded_attrs	Config-only invalid ID → ClickException
test_...verbose_echoes_config_exclusions	Verbose output with config-only exclusions

The untested path is the sub-clause inside if invalid_from_cli: where invalid_from_config is also non-empty:

if invalid_from_cli:
    msg = f"Invalid attribute ID(s): {', '.join(sorted(invalid_from_cli))}."
    if invalid_from_config:   # ← this sub-clause has no test
        msg += f" Also invalid in config file: {', '.join(sorted(invalid_from_config))}."
    msg += f" Valid IDs: {', '.join(sorted(valid_ids))}"
    raise click.BadParameter(msg)

If this sub-clause is deleted or broken, no test will catch the regression.

Remediation:

# Add a 5th test for the mixed-source invalid ID case:
# def test_run_assessment_validates_mixed_source_invalid_attrs_issue_302
# Setup: config with "invalid_config_attr", CLI exclude=["invalid_cli_attr"]
# Assert: click.BadParameter raised
# Assert: "invalid_cli_attr" in error message
# Assert: "Also invalid in config file" in error message
# Assert: "invalid_config_attr" in error message
pytest tests/unit/cli/test_main.py::TestRunAssessmentCommand -v

---

🔵 Minor Issues - Low Priority

2. Verbose output does not distinguish exclusion source

Attribute: N/A (UX improvement, not a scored attribute)
Confidence: 65%
Score Impact: −0.0 points

The merged verbose message "Excluded N attribute(s): attr1, attr2" does not indicate whether each exclusion came from --exclude or excluded_attributes in config. A user who sees unexpected exclusions cannot determine the source without checking both. Noted in two prior review comments; still unaddressed.

Low priority — the fix is functionally correct; this is a polish item for a follow-up.

---

Summary

Category	Finding
Bug	None — the core fix is correct
Test coverage	1 code path without coverage (mixed-source invalid IDs)
Security	None — valid_ids check prevents arbitrary IDs from config
CLAUDE.md compliance	Conventional commit format ✅, >80% coverage target needs 1 more test

• Auto-Fix Candidates: 0 critical issues
• Manual Review: 1 major (untested combined-error path)
• Total Score Improvement Potential: +0.1 points if test gap closed
• AgentReady Assessment: Run agentready assess . after fixes to verify score

---

🤖 Generated with Claude Code

_{If this review was useful, react with 👍. Otherwise, react with 👎.}

[dbasunag]

Thank you for the quick fix @kami619 !

[github-actions]

Code Review: fix(cli): Honor excluded_attributes from config file

Overall: This is a well-targeted bug fix that correctly addresses the root cause. The implementation is sound and the regression tests are thorough. A few minor issues worth addressing before merge.

---

Issues

1. Dead Code Guard (minor)

src/agentready/cli/main.py line 273

config_exclusions = set(config.excluded_attributes) if config else set()

config is never None at this point — it is always assigned before this line (either via load_config() or Config.load_default() at line 248-250). The if config else set() branch is unreachable dead code. Replace with:

config_exclusions = set(config.excluded_attributes)

2. Untested Error Path (minor)

The mixed invalid IDs case — where both CLI and config have invalid IDs simultaneously — has a specific code path (lines 286-291) that is never exercised by the 4 new tests. Specifically, the if invalid_from_config: branch inside if invalid_from_cli: (lines 288-290) is untested. This is low-risk but worth a note.

3. Valid IDs Verbose in Error Messages (minor)

Error messages include the full sorted list of all valid attribute IDs (31+ items), e.g.:

Valid IDs: api_versioning, changelog_management, claude_md_file, code_complexity, ...

This can produce multi-line terminal output. Consider appending Run 'agentready list-attributes' to see valid IDs as an alternative, or truncating the list.

---

Strengths

• Correct fix: cli_exclusions | config_exclusions cleanly merges both sources without overriding either.
• Good UX: Using click.BadParameter for CLI-sourced errors vs click.ClickException for config-sourced errors is the right choice — BadParameter sets a non-zero exit code and formats the error differently, which is appropriate for CLI argument errors.
• Tests document the bug: Each test's docstring explains the exact failure mode it guards against. This is helpful for future maintainers.
• Modern mock API: call_args.args[0] is more robust than the positional call_args[0][0] form — good practice.
• E2E test corrected: Replacing the invalid attribute IDs (repomix_config, claude_md) with valid ones (openapi_specs, claude_md_file) fixes the previously broken test.
• One-round-trip error reporting: When both sources have invalid IDs, all errors are reported together rather than forcing the user to fix-and-retry twice.

---

Summary

The bug fix logic is correct and the tests cover the primary scenarios well. The three items above are all minor — the dead code guard is the easiest to clean up before merge.

[github-actions]

🎉 This PR is included in version 2.28.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀