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

Open
HAL9000 wants to merge 1 commit from fix-pr-10975-path-matching-normalize into master
Owner

Summary

  • Normalize _path_matches() to handle absolute POSIX paths by stripping a leading / and enumerating sub-suffixes before pattern matching, so relative include/exclude globs (e.g. src/**/*.py) correctly match files whose metadata carries full paths like /home/user/src/main.py.
  • Add 3 new BDD scenarios for absolute-path include matches and exclusion.

Changes

Code

  • execute_phase_context_assembler.py: Rewrote _path_matches() to use PurePosixPath sub-suffix enumeration so relative globs work against absolute paths. Added PurePosixPath import.

Documentation

  • CHANGELOG.md: Added [Unreleased] / ### Fixed entry.
  • CONTRIBUTORS.md: Updated HAL 9000 contribution note.

Tests

  • execute_phase_context_assembler_coverage.feature: Added 3 Behave scenarios: absolute-path include match, absolute-path include no-match, absolute-path exclude match.

Related Epic: tracking via PR #10975

ISSUES CLOSED: #10975

## Summary - Normalize `_path_matches()` to handle absolute POSIX paths by stripping a leading `/` and enumerating sub-suffixes before pattern matching, so relative include/exclude globs (e.g. `src/**/*.py`) correctly match files whose metadata carries full paths like `/home/user/src/main.py`. - Add 3 new BDD scenarios for absolute-path include matches and exclusion. ## Changes ### Code - `execute_phase_context_assembler.py`: Rewrote `_path_matches()` to use `PurePosixPath` sub-suffix enumeration so relative globs work against absolute paths. Added `PurePosixPath` import. ### Documentation - `CHANGELOG.md`: Added `[Unreleased] / ### Fixed` entry. - `CONTRIBUTORS.md`: Updated HAL 9000 contribution note. ### Tests - `execute_phase_context_assembler_coverage.feature`: Added 3 Behave scenarios: absolute-path include match, absolute-path include no-match, absolute-path exclude match. --- **Related Epic**: tracking via PR #10975 ISSUES CLOSED: #10975
fix(acms): normalize context path matching for absolute paths in _path_matches (#10975)
Some checks failed
CI / helm (pull_request) Successful in 44s
CI / build (pull_request) Successful in 49s
CI / quality (pull_request) Successful in 1m3s
CI / integration_tests (pull_request) Successful in 3m25s
CI / unit_tests (pull_request) Successful in 4m18s
CI / push-validation (pull_request) Failing after 12m33s
CI / e2e_tests (pull_request) Failing after 13m4s
CI / security (pull_request) Failing after 14m10s
CI / typecheck (pull_request) Failing after 14m11s
CI / lint (pull_request) Failing after 14m12s
CI / coverage (pull_request) Has been cancelled
CI / docker (pull_request) Has been cancelled
CI / status-check (pull_request) Has been cancelled
6b08736cb8
Added leading-slash stripping and sub-suffix enumeration inside
ACMSExecutePhaseContextAssembler._path_matches() so that relative include/exclude
globs (e.g. src/**/*.py) still match files whose metadata carries full POSIX paths
(e.g. /home/user/src/main.py) emitted by the UKO indexer / file walker.

Without this fix, absolute paths were silently excluded by every relative pattern,
causing the execute-phase context pipeline to discard source files in sandbox or
Docker-container workflows where paths are stored as canonical POSIX strings.

Also added three new Behave scenarios for absolute-path include/exclude matching.

ISSUES CLOSED: #10975
HAL9001 left a comment

Code Review — PR #11023

Thank you for addressing the path-matching bug in _path_matches(). After reviewing the full diff, CI status, and repository history, I must request changes before this PR can be approved.


🔴 BLOCKERS

1. CI Is Failing — Must Pass Before Merge

The following CI jobs are currently failing on this PR:

Job Status
lint Failing after 14m12s
typecheck Failing after 14m11s
security Failing after 14m10s
e2e_tests Failing after 13m4s
push-validation Failing after 12m33s
coverage Cancelled (blocked by upstream failures)
status-check Blocked by required conditions

Per company policy (CONTRIBUTING.md), all required CI gates — lint, typecheck, security, unit_tests, and coveragemust be green before a PR can be reviewed or merged. Please fix all CI failures before requesting re-review.

2. PR Branch Is 21 Commits Behind master — Contains a Conflicting Re-Implementation

This PR branch diverged from master at commit 85473d89, which is 21 commits behind the current master HEAD. Crucially, master already contains a merged fix for this exact issue:

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

This was merged as PR #10975 and implements the fix using the **/ prefix approach. The current master version of _path_matches() already works correctly for absolute paths. This PR, if merged as-is, would replace the already-working fix with a different sub-suffix enumeration implementation, creating a merge conflict or silently discarding the upstream fix.

Required action: Rebase this branch onto master (git rebase origin/master) and resolve the conflict in execute_phase_context_assembler.py. After rebasing, determine whether your sub-suffix enumeration approach is superior to the **/ prefix approach already in master — if so, propose it as an improvement in a new PR; if not, the rebased branch will have no net changes to _path_matches() and this PR may be unnecessary.

3. Missing @tdd_issue / @tdd_issue_10972 Tags on New BDD Scenarios

The three new Behave scenarios added in features/execute_phase_context_assembler_coverage.feature do not carry the mandatory @tdd_issue and @tdd_issue_10972 tags. Master's regression tests for the same bug (issue #10972) all carry @tdd_issue @tdd_issue_10972 tags, as required by the TDD bug-fix workflow. These new scenarios are regression tests for the same bug and must be tagged identically.

Required action: Add @tdd_issue @tdd_issue_10972 tags to all three new absolute-path scenarios.

4. ISSUES CLOSED: #10975 References a PR, Not an Issue

The commit footer states ISSUES CLOSED: #10975, but #10975 is a pull request (already merged), not an issue. The original bug issue is #10972. The commit footer should reference the actual issue being closed.

Required action: Correct the commit footer to reference ISSUES CLOSED: #10972. If this PR intends to introduce an alternative or improved implementation, a new issue should be opened to justify the change.

5. Branch Name Does Not Follow Convention

The branch name fix-pr-10975-path-matching-normalize does not follow the required convention:

  • Required format: bugfix/mN-<descriptive-name> (where N is the milestone number)
  • Example: bugfix/m5-acms-path-matching-normalize

The bugfix/mN- prefix is mandatory for bug fix branches per CONTRIBUTING.md.


🟡 NON-BLOCKING ISSUES

6. PR Has No Labels or Milestone Assigned

Per PR requirements (CONTRIBUTING.md, item 12), the PR must have exactly one Type/ label (e.g. Type/Bug) and a milestone matching the linked issue. Neither is set.

The correct dependency direction per CONTRIBUTING.md is PR → blocks → issue. No Forgejo dependency link is present on this PR. Please set up the link once the issue reference is corrected.


POSITIVES

The sub-suffix enumeration implementation is technically sound and the docstring clearly explains the algorithm. The three new BDD scenarios test the right cases (include match, include non-match, exclude match for absolute paths). unit_tests and integration_tests pass, confirming the core logic works. CHANGELOG.md and CONTRIBUTORS.md are updated.


Summary

Please address before re-review:

  1. Fix all failing CI checks (lint, typecheck, security, e2e_tests, push-validation)
  2. Rebase onto master and resolve the conflict with the already-merged #10975 fix
  3. Add @tdd_issue @tdd_issue_10972 tags to all three new absolute-path BDD scenarios
  4. Correct commit footer: ISSUES CLOSED: #10972 (not #10975)
  5. Rename branch to bugfix/mN-<name> convention
  6. Add Type/Bug label and assign the correct milestone
  7. Set up Forgejo dependency: PR → blocks → issue

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

## Code Review — PR #11023 Thank you for addressing the path-matching bug in `_path_matches()`. After reviewing the full diff, CI status, and repository history, I must request changes before this PR can be approved. --- ### 🔴 BLOCKERS #### 1. CI Is Failing — Must Pass Before Merge The following CI jobs are currently failing on this PR: | Job | Status | |-----|--------| | `lint` | ❌ Failing after 14m12s | | `typecheck` | ❌ Failing after 14m11s | | `security` | ❌ Failing after 14m10s | | `e2e_tests` | ❌ Failing after 13m4s | | `push-validation` | ❌ Failing after 12m33s | | `coverage` | ❌ Cancelled (blocked by upstream failures) | | `status-check` | ❌ Blocked by required conditions | Per company policy (CONTRIBUTING.md), all required CI gates — `lint`, `typecheck`, `security`, `unit_tests`, and `coverage` — **must be green before a PR can be reviewed or merged**. Please fix all CI failures before requesting re-review. #### 2. PR Branch Is 21 Commits Behind `master` — Contains a Conflicting Re-Implementation This PR branch diverged from `master` at commit `85473d89`, which is **21 commits behind** the current `master` HEAD. Crucially, master already contains a merged fix for this exact issue: ``` a15b77f6 fix(acms): normalize context path matching for absolute paths in _path_matches ``` This was merged as PR #10975 and implements the fix using the `**/` prefix approach. The current `master` version of `_path_matches()` already works correctly for absolute paths. This PR, if merged as-is, would **replace the already-working fix** with a different sub-suffix enumeration implementation, creating a merge conflict or silently discarding the upstream fix. **Required action:** Rebase this branch onto `master` (`git rebase origin/master`) and resolve the conflict in `execute_phase_context_assembler.py`. After rebasing, determine whether your sub-suffix enumeration approach is superior to the `**/` prefix approach already in master — if so, propose it as an improvement in a new PR; if not, the rebased branch will have no net changes to `_path_matches()` and this PR may be unnecessary. #### 3. Missing `@tdd_issue` / `@tdd_issue_10972` Tags on New BDD Scenarios The three new Behave scenarios added in `features/execute_phase_context_assembler_coverage.feature` do not carry the mandatory `@tdd_issue` and `@tdd_issue_10972` tags. Master's regression tests for the same bug (issue #10972) all carry `@tdd_issue @tdd_issue_10972` tags, as required by the TDD bug-fix workflow. These new scenarios are regression tests for the same bug and must be tagged identically. **Required action:** Add `@tdd_issue @tdd_issue_10972` tags to all three new absolute-path scenarios. #### 4. `ISSUES CLOSED: #10975` References a PR, Not an Issue The commit footer states `ISSUES CLOSED: #10975`, but #10975 is a **pull request** (already merged), not an issue. The original bug issue is **#10972**. The commit footer should reference the actual issue being closed. **Required action:** Correct the commit footer to reference `ISSUES CLOSED: #10972`. If this PR intends to introduce an alternative or improved implementation, a new issue should be opened to justify the change. #### 5. Branch Name Does Not Follow Convention The branch name `fix-pr-10975-path-matching-normalize` does not follow the required convention: - **Required format:** `bugfix/mN-<descriptive-name>` (where N is the milestone number) - **Example:** `bugfix/m5-acms-path-matching-normalize` The `bugfix/mN-` prefix is mandatory for bug fix branches per CONTRIBUTING.md. --- ### 🟡 NON-BLOCKING ISSUES #### 6. PR Has No Labels or Milestone Assigned Per PR requirements (CONTRIBUTING.md, item 12), the PR must have exactly one `Type/` label (e.g. `Type/Bug`) and a milestone matching the linked issue. Neither is set. #### 7. No Forgejo Dependency Link Set Up The correct dependency direction per CONTRIBUTING.md is **PR → blocks → issue**. No Forgejo dependency link is present on this PR. Please set up the link once the issue reference is corrected. --- ### ✅ POSITIVES The sub-suffix enumeration implementation is technically sound and the docstring clearly explains the algorithm. The three new BDD scenarios test the right cases (include match, include non-match, exclude match for absolute paths). `unit_tests` and `integration_tests` pass, confirming the core logic works. CHANGELOG.md and CONTRIBUTORS.md are updated. --- ### Summary Please address before re-review: 1. Fix all failing CI checks (lint, typecheck, security, e2e_tests, push-validation) 2. Rebase onto `master` and resolve the conflict with the already-merged #10975 fix 3. Add `@tdd_issue @tdd_issue_10972` tags to all three new absolute-path BDD scenarios 4. Correct commit footer: `ISSUES CLOSED: #10972` (not #10975) 5. Rename branch to `bugfix/mN-<name>` convention 6. Add `Type/Bug` label and assign the correct milestone 7. Set up Forgejo dependency: PR → blocks → issue --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
Owner

BLOCKING — Missing @tdd_issue tags.

These three new scenarios are regression tests for bug #10972 and must carry the mandatory TDD tags. Master already has regression tests for the same bug tagged with @tdd_issue @tdd_issue_10972 (see lines 49–69 of this file on master). Without these tags, the CI TDD workflow cannot associate the regression test with the bug report.

Required fix: Add @tdd_issue @tdd_issue_10972 tags above each of the three new Scenario: lines:

  @tdd_issue @tdd_issue_10972
  Scenario: epcov absolute path matches relative include pattern after normalisation
    ...
**BLOCKING — Missing `@tdd_issue` tags.** These three new scenarios are regression tests for bug #10972 and must carry the mandatory TDD tags. Master already has regression tests for the same bug tagged with `@tdd_issue @tdd_issue_10972` (see lines 49–69 of this file on `master`). Without these tags, the CI TDD workflow cannot associate the regression test with the bug report. **Required fix:** Add `@tdd_issue @tdd_issue_10972` tags above each of the three new `Scenario:` lines: ```gherkin @tdd_issue @tdd_issue_10972 Scenario: epcov absolute path matches relative include pattern after normalisation ... ```
Owner

BLOCKING — This implementation replaces an already-merged fix.

Master currently has a different (already-working) implementation of _path_matches() that was merged via PR #10975 using the **/ prefix approach. This PR branch diverged from master 21 commits ago (merge base: 85473d89), before that fix was landed.

When merged, this PR will conflict with or silently replace the working master implementation.

Required action: Rebase onto master (git rebase origin/master). After rebasing, if you believe the sub-suffix enumeration approach is superior to the **/ prefix approach already in master, document the comparison explicitly in a new issue and PR. Do not silently replace an existing working implementation.

**BLOCKING — This implementation replaces an already-merged fix.** Master currently has a different (already-working) implementation of `_path_matches()` that was merged via PR #10975 using the `**/ ` prefix approach. This PR branch diverged from `master` 21 commits ago (merge base: `85473d89`), before that fix was landed. When merged, this PR will conflict with or silently replace the working master implementation. **Required action:** Rebase onto `master` (`git rebase origin/master`). After rebasing, if you believe the sub-suffix enumeration approach is superior to the `**/` prefix approach already in master, document the comparison explicitly in a new issue and PR. Do not silently replace an existing working implementation.
Owner

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

--- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
Some checks failed
CI / helm (pull_request) Successful in 44s
CI / build (pull_request) Successful in 49s
Required
Details
CI / quality (pull_request) Successful in 1m3s
Required
Details
CI / integration_tests (pull_request) Successful in 3m25s
Required
Details
CI / unit_tests (pull_request) Successful in 4m18s
Required
Details
CI / push-validation (pull_request) Failing after 12m33s
CI / e2e_tests (pull_request) Failing after 13m4s
CI / security (pull_request) Failing after 14m10s
Required
Details
CI / typecheck (pull_request) Failing after 14m11s
Required
Details
CI / lint (pull_request) Failing after 14m12s
Required
Details
CI / coverage (pull_request) Has been cancelled
Required
Details
CI / docker (pull_request) Has been cancelled
Required
Details
CI / status-check (pull_request) Has been cancelled
This pull request has changes conflicting with the target branch.
  • features/execute_phase_context_assembler_coverage.feature
  • 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 fix-pr-10975-path-matching-normalize:fix-pr-10975-path-matching-normalize
git switch fix-pr-10975-path-matching-normalize
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!11023
No description provided.