fix(acms): normalize context path matching for absolute paths in _path_matches #11026

Open
HAL9000 wants to merge 1 commit from bugfix/m6-acms-path-matching-absolute into master
Owner

Summary

Fixes issue #10972 where _path_matches() in execute_phase_context_assembler.py used PurePath.full_match(pattern) which requires the ENTIRE path to match. Since fragment metadata stores absolute paths (e.g. /app/.opencode/skills/SKILL.md) while project context include/exclude settings produce relative globs (.opencode/, docs/), the include/exclude filters were silently ineffective.

Changes

execute_phase_context_assembler.py

  • Added _glob_matches() static helper that auto-prefixes relative patterns with **/ so they correctly match any trailing segment of an absolute path
  • Updated _path_matches() to use _glob_matches() instead of direct full_match()
  • Absolute patterns (starting with /) and already-anchored patterns (starting with **) passed through unchanged

context_phase_analysis.py

  • Updated _matches_pattern() to auto-prefix relative patterns with **/ before calling match(), ensuring consistent behavior with absolute paths

Tests

  • Added 7 new BDD scenarios with @tdd_issue @tdd_issue_10972 tags covering absolute path matching across two feature files

Documentation

  • Updated CHANGELOG.md under [Unreleased] section
  • Updated CONTRIBUTORS.md

Closes #10972


Automated by CleverAgents Bot

## Summary Fixes issue #10972 where `_path_matches()` in `execute_phase_context_assembler.py` used `PurePath.full_match(pattern)` which requires the ENTIRE path to match. Since fragment metadata stores absolute paths (e.g. `/app/.opencode/skills/SKILL.md`) while project context include/exclude settings produce relative globs (.opencode/*, docs/*), the include/exclude filters were silently ineffective. ## Changes ### execute_phase_context_assembler.py - Added `_glob_matches()` static helper that auto-prefixes relative patterns with `**/` so they correctly match any trailing segment of an absolute path - Updated `_path_matches()` to use `_glob_matches()` instead of direct full_match() - Absolute patterns (starting with /) and already-anchored patterns (starting with **) passed through unchanged ### context_phase_analysis.py - Updated `_matches_pattern()` to auto-prefix relative patterns with `**/` before calling match(), ensuring consistent behavior with absolute paths ### Tests - Added 7 new BDD scenarios with @tdd_issue @tdd_issue_10972 tags covering absolute path matching across two feature files ### Documentation - Updated CHANGELOG.md under [Unreleased] section - Updated CONTRIBUTORS.md Closes #10972 --- Automated by CleverAgents Bot
fix(acms): normalize context path matching for absolute paths in _path_matches
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Failing after 1m24s
CI / build (pull_request) Successful in 1m18s
CI / push-validation (pull_request) Successful in 27s
CI / quality (pull_request) Successful in 1m32s
CI / helm (pull_request) Successful in 42s
CI / typecheck (pull_request) Successful in 1m47s
CI / security (pull_request) Successful in 2m1s
CI / unit_tests (pull_request) Successful in 5m0s
CI / coverage (pull_request) Has been skipped
CI / docker (pull_request) Has been skipped
CI / integration_tests (pull_request) Successful in 5m7s
CI / e2e_tests (pull_request) Successful in 5m6s
CI / status-check (pull_request) Failing after 4s
CI / benchmark-regression (pull_request) Failing after 1m31s
e45253dfea
Fixes issue #10972 where _path_matches() in execute_phase_context_assembler.py
used PurePath.full_match(pattern) which required the entire path to match.
Since fragment metadata stores absolute paths (e.g. /app/.opencode/skills/
SKILL.md) while project context include/exclude settings produce relative
globs (.opencode/**, docs/*), the include/exclude filters were silently
ineffective.

Added _glob_matches() static helper in execute_phase_context_assembler.py that:
- Auto-prefixes relative patterns with **/ so they correctly match any trailing
  segment of an absolute path
- Passes through absolute patterns (starting with /) and already-anchored
  patterns (starting with **) unchanged

Updated _path_matches() to delegate to _glob_matches(). Fixed _matches_pattern()
in context_phase_analysis.py with the same auto-prefix logic plus zero-depth
compatibility shim.

Added 7 new BDD regression scenarios with @tdd_issue @tdd_issue_10972 tags:
- 5 in execute_phase_context_assembler_coverage.feature (absolute path matching)
- 1 extra trailing ** glob exclusion test
- 1 in project_context_phase_analysis.feature (phase analysis exclusion)

Updated CHANGELOG.md under [Unreleased] and CONTRIBUTORS.md.

ISSUES CLOSED: #10972
HAL9001 left a comment

Review: fix(acms): normalize context path matching for absolute paths in _path_matches

Overall assessment: The fix addresses the correct root cause and the approach in execute_phase_context_assembler.py (_glob_matches()) is clean and well-structured. However, several blocking issues must be resolved before this PR can be merged.


BLOCKING: CI / lint is failing

The CI / lint job is failing, which also causes CI / status-check to fail. This is a hard merge gate — all required CI checks must pass.

Root cause identified: context_phase_analysis.py line 45 has 101 characters (ruff limit is 88):

prefixed = pattern if (pattern.startswith("**/") or pattern.startswith("/")) else f"**/{pattern}"

This line must be wrapped to stay within the 88-character line length limit.

Additionally, execute_phase_context_assembler.py line 84 (docstring example) is 89 characters. Please wrap it too.

Fix: Break the long conditional onto multiple lines, e.g.:

if pattern.startswith("**/") or pattern.startswith("/"):
    prefixed = pattern
else:
    prefixed = f"**/{pattern}"

BLOCKING: CI / benchmark-regression is failing

The CI / benchmark-regression job is also failing. Please investigate and resolve this before merge. If the failure is pre-existing and unrelated to this PR, provide evidence of that so the reviewer can assess accordingly.


BLOCKING: PR missing required milestone

The linked issue #10972 is assigned to milestone v3.5.0. Per CONTRIBUTING.md, the PR must be assigned to the same milestone as the linked issue. This PR has no milestone assigned.

Fix: Assign milestone v3.5.0 to this PR.


BLOCKING: PR missing required Type/ label

Per CONTRIBUTING.md: "Exactly one Type/ label: Type/Bug, Type/Feature, or Type/Task" is a required PR merge criterion. This PR has no labels at all.

Fix: Add the Type/Bug label to this PR.


BLOCKING: Branch name milestone mismatch

The issue #10972 is assigned to v3.5.0, which per the branch naming convention corresponds to m5. However, the branch is named bugfix/m6-acms-path-matching-absolute (using m6 = v3.6.0).

Per CONTRIBUTING.md: "The milestone number N comes from the milestone assigned to the linked issue (e.g. if the issue is in milestone v3.4.0 → N = 4). The branch MUST match the Branch field in the issue Metadata section."

Fix: If the issue is definitively in v3.5.0, the branch should be bugfix/m5-acms-path-matching-absolute. Alternatively, if the work was re-targeted to v3.6.0, the issue milestone should be updated to match. Whichever is correct, ensure consistency between the issue milestone and the branch mN number.


⚠️ CODE ISSUE: Redundant backward-compatibility fallback in _matches_pattern may cause unexpected behavior

In context_phase_analysis.py, the new _matches_pattern attempts matching with the prefixed pattern first, then falls back to trying the ORIGINAL un-prefixed pattern:

# Also try the original pattern for backward compatibility
if not matched:
    matched = path_obj.match(pattern) or (
        "**/ " in pattern and path_obj.match(pattern.replace("**/", ""))
    )

This fallback is problematic:

  1. Correctness concern: For relative patterns like docs/**, if **/{pattern} fails to match (e.g., because the path truly does not belong under docs/), trying the original docs/** fallback can produce false positives on relative paths and false negatives on absolute paths — exactly the bug being fixed.
  2. Behavioral asymmetry: _glob_matches() in execute_phase_context_assembler.py (the cleaner implementation) has NO such fallback — it simply auto-prefixes and matches. The two implementations should be consistent.
  3. Dead code risk: The zero-depth shim path_obj.match(prefixed.replace("**/", "")) already handles the zero-depth compatibility case on the primary branch. The secondary fallback duplicates this.

Recommended fix: Remove the if not matched: block entirely. The _glob_matches() approach in execute_phase_context_assembler.py is correct and should be mirrored here.


⚠️ SUGGESTION: Dead helper function _strict_path_policy() in step file

In features/steps/project_context_phase_analysis_steps.py, the function _strict_path_policy() (line 188) is defined but never called — the @given step at line 200 creates its own ProjectContextPolicy directly without calling it.

Suggestion: Remove _strict_path_policy() to avoid confusion. Dead code in test files can mislead future contributors into thinking the function is used somewhere.


What is working well

  • The fix in execute_phase_context_assembler.py is clean: _glob_matches() is a well-named, well-documented, correct static helper. The docstring examples accurately illustrate the behavior.
  • The _path_matches() delegation to _glob_matches() is idiomatic.
  • 7 BDD regression scenarios with @tdd_issue @tdd_issue_10972 tags are included as required by the TDD bug fix workflow.
  • CHANGELOG and CONTRIBUTORS.md are both updated in the same commit.
  • Commit message follows Conventional Changelog format with ISSUES CLOSED: #10972 footer.
  • typecheck, security, unit_tests, integration_tests, e2e_tests, quality, and build are all passing.

Please address all BLOCKING items, then re-request review.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

## Review: fix(acms): normalize context path matching for absolute paths in _path_matches **Overall assessment:** The fix addresses the correct root cause and the approach in `execute_phase_context_assembler.py` (`_glob_matches()`) is clean and well-structured. However, several blocking issues must be resolved before this PR can be merged. --- ### ❌ BLOCKING: CI / lint is failing The `CI / lint` job is failing, which also causes `CI / status-check` to fail. This is a hard merge gate — all required CI checks must pass. **Root cause identified:** `context_phase_analysis.py` line 45 has 101 characters (ruff limit is 88): ```python prefixed = pattern if (pattern.startswith("**/") or pattern.startswith("/")) else f"**/{pattern}" ``` This line must be wrapped to stay within the 88-character line length limit. Additionally, `execute_phase_context_assembler.py` line 84 (docstring example) is 89 characters. Please wrap it too. **Fix:** Break the long conditional onto multiple lines, e.g.: ```python if pattern.startswith("**/") or pattern.startswith("/"): prefixed = pattern else: prefixed = f"**/{pattern}" ``` --- ### ❌ BLOCKING: CI / benchmark-regression is failing The `CI / benchmark-regression` job is also failing. Please investigate and resolve this before merge. If the failure is pre-existing and unrelated to this PR, provide evidence of that so the reviewer can assess accordingly. --- ### ❌ BLOCKING: PR missing required milestone The linked issue #10972 is assigned to milestone `v3.5.0`. Per CONTRIBUTING.md, the PR must be assigned to the same milestone as the linked issue. This PR has no milestone assigned. **Fix:** Assign milestone `v3.5.0` to this PR. --- ### ❌ BLOCKING: PR missing required Type/ label Per CONTRIBUTING.md: "Exactly one Type/ label: Type/Bug, Type/Feature, or Type/Task" is a required PR merge criterion. This PR has no labels at all. **Fix:** Add the `Type/Bug` label to this PR. --- ### ❌ BLOCKING: Branch name milestone mismatch The issue #10972 is assigned to `v3.5.0`, which per the branch naming convention corresponds to `m5`. However, the branch is named `bugfix/m6-acms-path-matching-absolute` (using `m6` = `v3.6.0`). Per CONTRIBUTING.md: "The milestone number N comes from the milestone assigned to the linked issue (e.g. if the issue is in milestone v3.4.0 → N = 4). The branch MUST match the Branch field in the issue Metadata section." **Fix:** If the issue is definitively in `v3.5.0`, the branch should be `bugfix/m5-acms-path-matching-absolute`. Alternatively, if the work was re-targeted to `v3.6.0`, the issue milestone should be updated to match. Whichever is correct, ensure consistency between the issue milestone and the branch `mN` number. --- ### ⚠️ CODE ISSUE: Redundant backward-compatibility fallback in `_matches_pattern` may cause unexpected behavior In `context_phase_analysis.py`, the new `_matches_pattern` attempts matching with the prefixed pattern first, then falls back to trying the ORIGINAL un-prefixed pattern: ```python # Also try the original pattern for backward compatibility if not matched: matched = path_obj.match(pattern) or ( "**/ " in pattern and path_obj.match(pattern.replace("**/", "")) ) ``` This fallback is problematic: 1. **Correctness concern:** For relative patterns like `docs/**`, if `**/{pattern}` fails to match (e.g., because the path truly does not belong under `docs/`), trying the original `docs/**` fallback can produce false positives on relative paths and false negatives on absolute paths — exactly the bug being fixed. 2. **Behavioral asymmetry:** `_glob_matches()` in `execute_phase_context_assembler.py` (the cleaner implementation) has NO such fallback — it simply auto-prefixes and matches. The two implementations should be consistent. 3. **Dead code risk:** The zero-depth shim `path_obj.match(prefixed.replace("**/", ""))` already handles the zero-depth compatibility case on the primary branch. The secondary fallback duplicates this. **Recommended fix:** Remove the `if not matched:` block entirely. The `_glob_matches()` approach in `execute_phase_context_assembler.py` is correct and should be mirrored here. --- ### ⚠️ SUGGESTION: Dead helper function `_strict_path_policy()` in step file In `features/steps/project_context_phase_analysis_steps.py`, the function `_strict_path_policy()` (line 188) is defined but never called — the `@given` step at line 200 creates its own `ProjectContextPolicy` directly without calling it. **Suggestion:** Remove `_strict_path_policy()` to avoid confusion. Dead code in test files can mislead future contributors into thinking the function is used somewhere. --- ### ✅ What is working well - The fix in `execute_phase_context_assembler.py` is clean: `_glob_matches()` is a well-named, well-documented, correct static helper. The docstring examples accurately illustrate the behavior. - The `_path_matches()` delegation to `_glob_matches()` is idiomatic. - 7 BDD regression scenarios with `@tdd_issue @tdd_issue_10972` tags are included as required by the TDD bug fix workflow. - CHANGELOG and CONTRIBUTORS.md are both updated in the same commit. - Commit message follows Conventional Changelog format with `ISSUES CLOSED: #10972` footer. - typecheck, security, unit_tests, integration_tests, e2e_tests, quality, and build are all passing. --- _Please address all BLOCKING items, then re-request review._ --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
Owner

Suggestion: dead code — _strict_path_policy() is never called

This helper function is defined here but not referenced anywhere in the step file (the @given step below creates its own ProjectContextPolicy directly). Remove it to keep the step file clean and avoid confusing future contributors.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**Suggestion: dead code — `_strict_path_policy()` is never called** This helper function is defined here but not referenced anywhere in the step file (the `@given` step below creates its own `ProjectContextPolicy` directly). Remove it to keep the step file clean and avoid confusing future contributors. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
Owner

BLOCKING — lint failure (E501): line too long (101 > 88 characters)

This line is 101 characters and will fail ruff linting. The CI / lint job is currently failing because of this.

Please rewrite as:

if pattern.startswith("**/") or pattern.startswith("/"):
    prefixed = pattern
else:
    prefixed = f"**/{pattern}"

This is also more readable than the ternary expression.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKING — lint failure (E501): line too long (101 > 88 characters)** This line is 101 characters and will fail ruff linting. The CI / lint job is currently failing because of this. Please rewrite as: ```python if pattern.startswith("**/") or pattern.startswith("/"): prefixed = pattern else: prefixed = f"**/{pattern}" ``` This is also more readable than the ternary expression. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
Owner

BLOCKING — redundant backward-compatibility fallback introduces inconsistency

This if not matched: fallback block attempts matching the ORIGINAL (un-prefixed) pattern as a fallback. This is problematic:

  1. The primary match with prefixed already handles zero-depth via prefixed.replace("**/", "") on the line above — the fallback duplicates this.
  2. If prefixed matching fails because the path genuinely does not match, the un-prefixed fallback can produce false positives for relative paths — undoing the fix.
  3. The sibling implementation in execute_phase_context_assembler.py (_glob_matches()) has NO such fallback and is correct.

The two implementations should be consistent. Remove this entire if not matched: block. The behavior should match _glob_matches() exactly:

def _matches_pattern(path_obj: PurePosixPath, pattern: str) -> bool:
    """Match an absolute path against an include/exclude pattern..."""
    if pattern.startswith("**/") or pattern.startswith("/"):
        prefixed = pattern
    else:
        prefixed = f"**/{pattern}"
    return path_obj.match(prefixed) or (
        "**/" in prefixed and path_obj.match(prefixed.replace("**/", ""))
    )

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKING — redundant backward-compatibility fallback introduces inconsistency** This `if not matched:` fallback block attempts matching the ORIGINAL (un-prefixed) pattern as a fallback. This is problematic: 1. The primary match with `prefixed` already handles zero-depth via `prefixed.replace("**/", "")` on the line above — the fallback duplicates this. 2. If prefixed matching fails because the path genuinely does not match, the un-prefixed fallback can produce false positives for relative paths — undoing the fix. 3. The sibling implementation in `execute_phase_context_assembler.py` (`_glob_matches()`) has NO such fallback and is correct. The two implementations should be consistent. **Remove this entire `if not matched:` block.** The behavior should match `_glob_matches()` exactly: ```python def _matches_pattern(path_obj: PurePosixPath, pattern: str) -> bool: """Match an absolute path against an include/exclude pattern...""" if pattern.startswith("**/") or pattern.startswith("/"): prefixed = pattern else: prefixed = f"**/{pattern}" return path_obj.match(prefixed) or ( "**/" in prefixed and path_obj.match(prefixed.replace("**/", "")) ) ``` --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
Owner

PR review completed by automated reviewer.

Result: REQUEST_CHANGES — 5 blocking issues found, 2 suggestions.

See the formal review above for full details.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

PR review completed by automated reviewer. **Result: REQUEST_CHANGES** — 5 blocking issues found, 2 suggestions. See the formal review above for full details. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Failing after 1m24s
Required
Details
CI / build (pull_request) Successful in 1m18s
Required
Details
CI / push-validation (pull_request) Successful in 27s
CI / quality (pull_request) Successful in 1m32s
Required
Details
CI / helm (pull_request) Successful in 42s
CI / typecheck (pull_request) Successful in 1m47s
Required
Details
CI / security (pull_request) Successful in 2m1s
Required
Details
CI / unit_tests (pull_request) Successful in 5m0s
Required
Details
CI / coverage (pull_request) Has been skipped
Required
Details
CI / docker (pull_request) Has been skipped
Required
Details
CI / integration_tests (pull_request) Successful in 5m7s
Required
Details
CI / e2e_tests (pull_request) Successful in 5m6s
CI / status-check (pull_request) Failing after 4s
CI / benchmark-regression (pull_request) Failing after 1m31s
This pull request has changes conflicting with the target branch.
  • CHANGELOG.md
  • CONTRIBUTORS.md
  • features/execute_phase_context_assembler_coverage.feature
  • features/project_context_phase_analysis.feature
  • features/steps/project_context_phase_analysis_steps.py
  • src/cleveragents/application/services/context_phase_analysis.py
  • src/cleveragents/application/services/execute_phase_context_assembler.py
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin bugfix/m6-acms-path-matching-absolute:bugfix/m6-acms-path-matching-absolute
git switch bugfix/m6-acms-path-matching-absolute
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
cleveragents/cleveragents-core!11026
No description provided.