Skip to content

feat: add directory-scoped matching with trailing-slash patterns and directory queries#119

Merged
gregology merged 9 commits intomainfrom
dir-scoped-matching
Mar 24, 2026
Merged

feat: add directory-scoped matching with trailing-slash patterns and directory queries#119
gregology merged 9 commits intomainfrom
dir-scoped-matching

Conversation

@gregology
Copy link
Copy Markdown
Owner

The problem

All matching in sctx is file-oriented. Resolve takes a file path, matches it against glob patterns, and returns results. There's no way to ask "what decisions apply to src/api/?" without naming a specific file inside it.

This matters because decisions and context are often about directories, not individual files. "We chose REST over GraphQL" applies to src/api/ as a whole. An agent planning work there needs that context before it opens any file. But today the only way to surface it is to query for a specific file like src/api/handler.py and hope the globs work out.

There's a second gap: you can't scope an entry to a directory without it leaking into subdirectories. match: ["tests/**"] matches everything under tests/ recursively. If a decision only applies to the tests/ directory itself (say, "fixtures live in conftest.py at this level, don't duplicate them in subdirectories"), there's no way to express that.

Both gaps also block future features like skills, which are inherently directory-scoped.

The approach: trailing-slash convention

A match pattern ending with / targets a directory. match: ["tests/"] applies to the tests/ directory but not to tests/unit/ and not to files inside tests/. This follows the same convention as .gitignore and rsync, so it's already familiar.

Full glob syntax works in directory patterns. match: ["foo/**/tests/"] matches any tests/ directory anywhere under foo/. match: ["**/api/"] matches any directory named api/ at any depth.

For the resolution engine, ResolveRequest gets a DirPath field (mutually exclusive with FilePath). When set, discovery starts from the directory itself instead of its parent, and the matching logic handles both trailing-slash patterns and file-glob patterns against the directory.

The CLI detects directory arguments automatically. sctx decisions src/api/ does a directory query. sctx decisions src/api/handler.py does a file query. No new flags needed.

The tricky part: file-glob patterns in directory queries

When you query a directory, trailing-slash patterns are straightforward: does the directory match or not. But file-glob patterns like **/*.py or src/api/handlers/*.py need a different question: could this pattern produce matches inside the queried directory?

The first attempt used synthetic file paths. Append /_ and /_/_ to the directory and test the pattern against those. This works for wildcards (**/*.py matches src/_) but falls apart for fixed path components. src/api/handlers/*.py can't match src/_ because _ doesn't equal api.

The approach that works is segment-by-segment walking. Split the pattern and directory path by / and walk them together, handling *, **, and literal segments. If the directory is consumed before the pattern, remaining pattern segments could match files inside. If the pattern is consumed before the directory, the pattern doesn't reach deep enough. If ** appears, it can bridge any gap.

One subtle case: foo/* with directory foo/bar. The walk consumes both foo->foo and *->bar, exhausting the entire pattern. Since there's nothing left to match files inside foo/bar/, the pattern doesn't match that directory. But foo/* does match foo/ because after consuming foo->foo, the * segment remains to match files directly in foo/.

Alternatives considered

Synthetic file paths only. Append fake filenames to the directory and test patterns against them. Simple to implement but fundamentally broken for patterns with fixed path components deeper than a couple levels. We'd need to generate synthetic paths matching every possible directory structure the pattern could traverse, which is unbounded.

Separate dir_match field. A dedicated field for directory patterns, keeping match for files only. More explicit, but doubles the matching surface in every entry type. The trailing-slash convention keeps one field and is already well-understood.

Convention only (no trailing slash, no directory queries). Put decisions in the directory's own AGENTS.yaml and let file queries pick them up. Works for simple cases but can't express "this applies to foo/ but not foo/bar/." You'd need an AGENTS.yaml in every subdirectory just to exclude things, which inverts the problem.

Regex instead of globs. More expressive but a worse developer experience. Nobody wants to write ^src/api/handlers/[^/]*\.py$ when src/api/handlers/*.py does the same thing.

@gregology
Copy link
Copy Markdown
Contributor

gregology bot commented Mar 24, 2026

Review: directory-scoped matching

Nice feature — the trailing-slash convention is a good call and the PR description is genuinely excellent. But I found two bugs in the segment-walking logic that need fixing before merge.


Critical

matchSegments returns wrong result when ** is the last pattern segmentinternal/core/engine.go:371

Pattern src/** against directory src/api/ returns false. The walk matches src -> src, then ** tries consuming api — the recursive call lands at pi=2, di=2, and pi < len(pat) evaluates to 2 < 2 = false. But src/** obviously matches files like src/api/handler.py, so directory src/api/ should match.

I wrote a quick test to confirm — src/** against src/api/ and src/api/handlers/ both return 0 entries.

Fix: when ** is the last segment, it can match remaining dir segments and files inside, so short-circuit:

if p == "**" {
    if pi == len(pat)-1 {
        return true
    }
    for skip := 0; skip <= len(dir)-di; skip++ {
        if matchSegments(pat, dir, pi+1, di+skip) {
            return true
        }
    }
    return false
}

**/ prefix shortcut breaks exclude patterns for directory queriesinternal/core/engine.go:340

fileGlobMatchesDir returns true for all directories when a pattern starts with **/. This is defensible for match (showing extra context is acceptable), but matchesDirGlobs runs the same anyDirPatternMatches pipeline for excludes too. So exclude: ["**/vendor/**"] excludes every single directory. Confirmed this — an entry with that exclude returns 0 results for src/.

This is a pretty common pattern people will write. I think the cleanest fix is to drop the **/ shortcut entirely and let everything go through matchSegments (after fixing the trailing-** bug above). The segment walker already handles ** properly for patterns like **/vendor/** — after ** consumes src, the literal vendor fails to match, so src/ correctly isn't excluded. The only truly "matches everything" cases (** and **/*) already have their own fast paths.


Warnings

Missing test coverage for both bugs aboveinternal/core/engine_test.go

TestResolve_DirQuery_FileGlobMatchesContainingDir tests src/** against src/ (one level deep) but not src/api/ (two levels). That's why the trailing-** bug slipped through. And there's no test for exclude with **/-prefixed file globs in directory queries.

Minimum additions:

  • match: ["src/**"] queried against src/api/ and src/api/handlers/ — both should match
  • exclude: ["**/vendor/**"] queried against src/ — should NOT be excluded
  • exclude: ["**/vendor/**"] queried against vendor/ — should be excluded

isDir silently changes query type based on filesystem statecmd/sctx/main.go:336

os.Stat runs first, so sctx decisions src/api does a directory query when the dir exists but a file query when it doesn't (typo, wrong cwd, etc). No error, no warning, just different behavior. Might be worth either warning on stderr when the path doesn't exist, or documenting that trailing / is the reliable way to force a directory query.


Notes

errMutuallyExclusive uses fmt.Errorf with no format verbsinternal/core/engine.go:13

All the other sentinel errors in the project use errors.New. Small thing but easy to fix for consistency.


Overall

The design is solid and the PR description is one of the better ones I've read — the "tricky part" section explaining the segment-walking approach and the foo/* vs foo/bar subtlety is great. The two bugs above are blockers though. The trailing-** fix is straightforward, and I think dropping the **/ shortcut in favor of the segment walker handles both issues cleanly.

Developer metrics

Total duration: 6m 26s
Turns: 45
Tool calls: 32
Tokens: 877,941 input / 14,850 output

Stage Model Duration Turns Tool calls Tokens (in/out) Cache read Cache creation
analyze claude-opus-4-6 2m 17s 27 23 200,950 / 2,309 161,099 39,841
review_findings claude-opus-4-6 3m 41s 16 8 539,012 / 11,392 478,779 60,223
draft claude-opus-4-6 0m 27s 2 1 137,979 / 1,149 74,691 63,285

@gregology
Copy link
Copy Markdown
Contributor

gregology bot commented Mar 24, 2026

Review: directory-scoped matching

This PR adds directory queries to the resolution engine via trailing-slash patterns and a new DirPath field on ResolveRequest. The segment-walking algorithm for matching file-glob patterns against directories is the interesting part. Docs are thorough, tests are solid overall — but there are two matching bugs worth fixing before merge.


Critical

fileGlobMatchesDir breaks excludes at the source directoryinternal/core/engine.go:380

When relDir == "" (querying the directory that contains the AGENTS.yaml), fileGlobMatchesDir short-circuits to return true for every pattern. That's fine for match evaluation — any pattern could match files here. But this same function gets called for exclude patterns through matchesDirGlobs -> anyDirPatternMatches -> dirPatternMatches -> fileGlobMatchesDir. So exclude: ["vendor/**"] will exclude the root directory itself, because fileGlobMatchesDir("vendor/**", "") returns true.

I wrote a test to confirm:

// exclude: ["vendor/**"] with DirPath = tmpDir (the root)
// Expected: 1 entry (root should not be excluded)
// Actual: 0 entries (root is incorrectly excluded)

Fix options:

  • Pass a flag into fileGlobMatchesDir indicating match vs exclude context, and don't short-circuit for excludes when relDir == ""
  • Or handle it in matchesDirGlobs — when relDir == "", skip the exclude check for file-glob patterns (since a file-glob exclude can't meaningfully target the root without targeting everything)

Either way, add a test that queries tmpDir itself with a file-glob exclude.


Warnings

matchSegments false negatives for ancestor directoriesinternal/core/engine.go:456

The literalMatched heuristic rejects valid matches when ** consumes all directory segments and the remaining pattern has multiple non-** segments. Concrete example: **/a/b/*.py does not match directory src/, even though src/a/b/foo.py is a valid match for the pattern.

Trace: pattern ["**", "a", "b", "*.py"] vs dir ["src"]. The ** consumes src (skip=1), leaving ["a", "b", "*.py"] with literalMatched=false. Lines 460-467 check len(remaining) == 1 (nope, it's 3) and remaining[0] == "**" (nope, it's "a"), so it returns false.

Same issue hits **/api/*.py against src/ — confirmed by test.

The simplest correct fix: when literalMatched is false, return true if len(remaining) > 0. The ** already bridged to this directory; remaining segments describe a subpath that could exist inside it. This is slightly less selective but semantically correct — the question is "could this pattern produce matches here" and the answer is yes.

Missing test coverage for relDir == "" excludesinternal/core/engine_test.go:1322

TestResolve_DirQuery_SelfDirectory only tests default match with no excludes. The exclude tests (ExcludeFileGlobPattern, ExcludeWithTrailingSlash) never query tmpDir. That's exactly the code path where the critical bug lives.

Missing test coverage for ancestor directory matchinginternal/core/engine_test.go:1700

TestResolve_DirQuery_DoubleStarPrefixMatchIsSelective tests src/api/ and src/models/ but not src/ or the root. Adding those cases would catch the matchSegments false-negative.


Notes

Filter function duplicationinternal/core/engine.go:193

filterContext/filterContextDir and filterDecisions/filterDecisionsDir are near-identical, differing only in which glob function they call. You could collapse these with a matchFn parameter. Not a big deal — the code reads fine as-is.

isDir stat behaviorcmd/sctx/main.go:348

isDir does an os.Stat first, so an existing directory always triggers a directory query even without a trailing slash. Reasonable behavior, just worth a line in the CLI docs saying "existing directories are always treated as directory queries; trailing slash is the fallback for paths that don't exist on disk."


The exclude-at-root bug is a blocker — it'll bite anyone with exclude patterns who queries a project root or the directory containing an AGENTS.yaml. The matchSegments false-negative is less urgent but worth fixing while you're in here. Everything else looks good — the trailing-slash convention is clean, the segment walker is well-thought-out, and the test suite is thorough (these two gaps aside).

Developer metrics

Total duration: 5m 29s
Turns: 46
Tool calls: 31
Tokens: 1,136,643 input / 13,394 output

Stage Model Duration Turns Tool calls Tokens (in/out) Cache read Cache creation
analyze claude-opus-4-6 2m 20s 21 17 194,784 / 4,874 159,536 35,238
review_findings claude-opus-4-6 2m 37s 24 13 789,998 / 7,256 725,491 64,487
draft claude-opus-4-6 0m 31s 1 1 151,861 / 1,264 83,926 67,932

@gregology
Copy link
Copy Markdown
Contributor

gregology bot commented Mar 24, 2026

Adds directory-scoped matching to the resolution engine — trailing-slash patterns like tests/ that target a directory without leaking into subdirectories, plus a DirPath field on ResolveRequest so callers can query directories directly. The segment-walking logic for determining whether file-glob patterns could match inside a directory is the interesting part.


Critical

Empty FilePath + empty DirPath silently resolves against cwd

internal/core/engine.go — When both fields are empty strings, Resolve falls through to resolveFile, where filepath.Abs("") quietly returns the working directory. A caller who constructs a ResolveRequest with just Action and Timing set gets results they never asked for instead of an error.

Fix is straightforward — add a guard after the mutual-exclusivity check:

if req.FilePath == "" && req.DirPath == "" {
    return nil, nil, errors.New("FilePath or DirPath is required")
}

Warnings

Match vs exclude asymmetry for file-glob patterns in directory queries

internal/core/engine.go — The same pattern means different things depending on which field it's in. match: ["**/vendor/**"] matches every directory (generous: vendor could be nested anywhere below). exclude: ["**/vendor/**"] only excludes directories that actually have vendor in their path (strict). This is a deliberate choice and the code comments explain the reasoning, but users are going to trip over it.

The TestResolve_DirQuery_ExcludeStrictVsMatchGenerous test documents the behavior well. I'd add a paragraph to docs/context.md under "Directory patterns" spelling out the asymmetry with a concrete example so people don't have to discover it experimentally.

Exponential recursion with multiple ** segments

internal/core/engine.go (matchSegments) — Pattern **/**/foo/*.py against directory bar: the first ** fans out via the skip loop, each branch hits the second ** and fans out again. This is O(n^k) in the number of ** segments. In practice the inputs are tiny so it won't blow up, but it's worth either:

  • Adding a targeted test with something like **/**/**/**/*.py against a 4-level directory to prove it stays fast
  • Or collapsing consecutive ** segments early (they're semantically equivalent to a single **)

matchSegmentsStrict branching is hard to follow

internal/core/engine.go — The literalMatched tracking and the special cases when di == len(dir) are correct on close inspection, but it took me a few passes to convince myself. A short truth table in a comment above the function would help a lot:

// Pattern     | Dir     | Result | Why
// *           | foo     | false  | pattern exhausted, no remaining segments
// foo/*       | foo     | true   | literal "foo" matched, * remains for children
// **/vendor/**| src     | false  | no literal matched, ** can't validate alone
// **/vendor/**| vendor  | true   | "vendor" literal matched, ** remains

Notes

Duplicate comment in test file

internal/core/engine_test.go — There's a merge artifact: // assertContextContents checks// assertContextContents checks that the matched context entries have exactly. Just needs the duplicate prefix removed.

Usage string not updated

cmd/sctx/main.go (line 17) — The usage constant still says "Query context entries for a file" and "Query decisions for a file". The descriptions further down in the file were updated to say "file or directory" but the top-level help text was missed.

Adapters don't support directory queries yet

internal/adapter/claude.go, internal/adapter/pi.go — Hook-based integration still only uses FilePath, so directory queries are CLI-only for now. The PR description mentions this enables future directory-scoped features like skills, so this seems like intentional staging. Might be worth a tracking issue or a note in the PR so it doesn't get lost.


One real bug (the empty-path fallthrough), a few things worth documenting better, and a minor typo. The core matching logic and test coverage are solid — the property-based fuzz test is a nice touch. Should be good to go after addressing the empty-path guard.

Developer metrics

Total duration: 3m 32s
Turns: 31
Tool calls: 23
Tokens: 770,763 input / 6,991 output

Stage Model Duration Turns Tool calls Tokens (in/out) Cache read Cache creation
analyze claude-opus-4-6 1m 20s 18 14 261,695 / 2,713 220,661 41,023
review_findings claude-opus-4-6 1m 36s 12 8 384,257 / 3,150 333,349 50,896
draft claude-opus-4-6 0m 35s 1 1 124,811 / 1,128 70,469 54,339

@gregology
Copy link
Copy Markdown
Contributor

gregology bot commented Mar 24, 2026

Adds directory-scoped matching to the resolution engine via a trailing-slash convention (match: ["tests/"]) and a new DirPath field on ResolveRequest. The CLI auto-detects directory arguments, and the core matching splits into generous (match) vs strict (exclude) segment-walking for file-glob patterns in directory queries. Clean design overall — the split between file and directory resolution paths is well-factored and the test coverage is thorough. A few things worth looking at:


Warnings

dirSlashPatternMatches mishandles wildcard patterns at source directory — internal/core/engine.go:382

When relDir is empty (querying the directory that contains the AGENTS.yaml), the code converts it to "./". This means doublestar.Match("*/", "./") succeeds because * matches .. So match: ["*/"] — which should mean "any immediate child directory" — incorrectly matches the source directory itself. Same problem on the exclude side: exclude: ["*/"] would incorrectly knock out the source directory.

I'd either special-case relDir=="" to only match explicit self-references like "./" or patterns starting with **/, or avoid the "./" representation entirely and handle self-matching separately. Either way, add a test for match: ["*/"] targeting the source directory (should not match) and exclude: ["*/"] (should not exclude).

**/-prefixed patterns match every directory — internal/core/engine.go:406

fileGlobMatchesDir short-circuits to return true for any pattern starting with **/. This means match: ["**/vendor/**"] will match src/, tests/, literally everything during directory queries. The test suite confirms this is intentional (generous matching), but it's pretty surprising behavior — someone writing match: ["**/vendor/**"] probably doesn't expect it to show up when querying src/.

Two options: either run **/-prefixed patterns through the same dirCouldContainMatch segment walker (which would still be generous but not unconditionally true), or at minimum add a note in context.md explaining that **/X/** in match surfaces context for all directories and recommending X/** for tighter scoping.

Missing test coverage for wildcard dir patterns at source root — internal/core/engine_test.go

TestResolve_DirQuery_SelfDirectory uses the default match: ["**"] which routes through fileGlobMatchesDir, never touching dirSlashPatternMatches. There's no test exercising match: ["*/"] or match: ["**/"] when the queried directory is the one containing the AGENTS.yaml. This is exactly where the "./" bug above lives.


Notes

Adapters don't support directory queries yet — internal/adapter/claude.go:82

Claude and Pi hook adapters still only pass FilePath. Directory queries are CLI-only for now. Makes sense to ship this way, but worth tracking — agents using hooks won't see trailing-slash-scoped entries until the adapters get updated.

Validator could warn on directory-only match lists — internal/validator/validate.go:224

If someone writes match: ["tests/"] with no file-glob patterns, that entry is completely invisible to file queries. The validator accepts it fine (syntactically valid) but could emit a warning like "all match patterns are directory-scoped; this entry will only appear in directory queries." Helps catch the tests/ vs tests/** typo.

Recursive backtracking in segment walkers — internal/core/engine.go:474

matchSegments and matchSegmentsStrict backtrack over ** segments with O(d^k) worst case (d = directory depth, k = number of non-consecutive **). collapseDoubleStars prevents the degenerate consecutive case. Real-world patterns are short and hand-written so this is fine in practice. The property-based fuzz test gives good confidence against panics here.


The core algorithm is well thought out and the generous-vs-strict asymmetry for match/exclude is a smart design choice. The "./" bug is the main thing I'd want fixed before merge — the rest are improvements that could land later.

Developer metrics

Total duration: 5m 28s
Turns: 39
Tool calls: 30
Tokens: 1,085,344 input / 12,944 output

Stage Model Duration Turns Tool calls Tokens (in/out) Cache read Cache creation
analyze claude-opus-4-6 2m 9s 33 25 590,806 / 3,513 526,277 64,508
review_findings claude-opus-4-6 2m 53s 5 4 322,188 / 8,350 247,089 75,090
draft claude-opus-4-6 0m 25s 1 1 172,350 / 1,081 94,262 78,085

@gregology
Copy link
Copy Markdown
Contributor

gregology bot commented Mar 24, 2026

Review: directory-scoped matching with trailing-slash patterns

Adds directory queries to the resolution engine — trailing-slash patterns like tests/ target directories without leaking into subdirectories, and ResolveRequest gets a new DirPath field. The segment-by-segment walker for matching file-glob patterns against directories is the interesting part, with a generous/strict asymmetry between match and exclude that's well thought out.


Critical

internal/core/engine.go:379./ pattern is silently broken

dirSlashPatternMatches with relDir="" only matches pure **/ chains. The **/ stripping loop doesn't touch ./, so trimmed = "./" which != "", and it returns false. This means match: ["./"] matches nothing — not directory queries (this bug), not file queries (isDirPattern skips it). I wrote a test against the branch and confirmed it.

This matters because docs/examples.md ships an example using exactly this pattern:

match: ["./"]  # scoped to this directory only, not inherited by subdirectories

Anyone copying that example gets silent failure.

Fix: handle ./ in the relDir=="" branch — either check trimmed == "./" after the loop, or normalize ./ away before it. Add a test for match: ["./"] at the source directory (should match) and at a child directory (should not).


Warnings

internal/core/engine.go:416**/ prefix short-circuit is very generous

fileGlobMatchesDir returns true for any pattern starting with **/. So match: ["**/vendor/**"] matches every directory in the project. The test suite validates this as intentional, and the generous-vs-strict design is clearly documented, but it could get noisy in real projects.

The segment walker would actually produce the same result for most of these patterns (since ** bridges any gap), so you could drop the short-circuit and route through dirCouldContainMatch without losing correctness. That would at least let patterns like **/vendor/*.py be slightly more precise. Not blocking, but worth considering.

internal/core/engine_test.go — no test for mixed pattern types in match vs exclude

There's no test where match has only dir patterns and exclude has only file-glob patterns (or the reverse). Something like match: ["tests/"], exclude: ["tests/**"] querying tests/ — the dir pattern matches, then the file-glob exclude goes through fileGlobExcludesDir which has different semantics. Would be good to verify this cross-type interaction works as expected.

internal/core/engine.go:530literalMatched is a misleading name

This variable is set to true when any non-** segment matches, including wildcards like *. Calling it literalMatched implies only fixed strings count. The doc comment gets it right ("non-** pattern segment"), but the variable name doesn't. segmentMatched or nonStarStarMatched would be clearer.


Notes

internal/adapter/claude.go — hook adapters don't support directory queries

The Claude and Pi adapters only use FilePath. Directory queries are CLI-only for now. The PR description mentions skills as a future use case that needs this, so maybe drop a TODO in the adapter code so it's discoverable later.

internal/validator/validate.go:224 — validator doesn't know about dir patterns

Trailing-slash patterns pass validation (they're syntactically valid globs), which is fine. Once the ./ engine bug is fixed this is a non-issue. Low priority.


The design here is solid — the trailing-slash convention is familiar, the generous/strict split for match vs exclude is a good call, and the test coverage is thorough (property-based fuzz test included). The ./ bug is the one blocker — it's a documented pattern that silently does nothing. Fix that and add the mixed-pattern-type test, and this looks good to merge.

Developer metrics

Total duration: 6m 54s
Turns: 56
Tool calls: 37
Tokens: 1,721,237 input / 15,825 output

Stage Model Duration Turns Tool calls Tokens (in/out) Cache read Cache creation
analyze claude-opus-4-6 1m 48s 24 19 321,674 / 2,684 276,538 45,124
review_findings claude-opus-4-6 4m 38s 31 17 1,238,817 / 12,106 1,167,082 71,710
draft claude-opus-4-6 0m 27s 1 1 160,746 / 1,035 86,131 74,612

@gregology
Copy link
Copy Markdown
Contributor

gregology bot commented Mar 24, 2026

Review: directory-scoped matching

Nice feature. The trailing-slash convention is a good call — familiar from .gitignore/rsync, keeps the schema simple, and the segment-walking approach for file-glob patterns in directory queries is well thought out. The test coverage is thorough, especially the table-driven DirSlashPatternsAtRoot test and the property-based fuzz test.

One real bug to fix before merging, plus a few smaller things.


Critical

internal/core/engine.go:567matchSegmentsStrict over-excludes with **/filename patterns

The single-remaining-segment exception returns true even when literalMatched is false. This means exclude: ["**/*.py"] will exclude every non-root directory in directory queries. I wrote a quick integration test to confirm: a context entry with exclude: ["**/*.py"] returns zero results for sctx context src/. That's wrong — the exclude targets Python files, not directories.

The root cause is here:

// No literals validated. Only match for single filename segment
// or remaining starting with **.
if len(remaining) == 1 {
    return true  // <-- should be: return literalMatched
}

return remaining[0] == "**"  // <-- should be: return literalMatched || remaining[0] == "**"

When ** consumes all directory segments and a single filename-like segment remains (e.g. *.py), the function says "yes, this excludes the directory" without any real directory segment having been validated. Patterns like vendor/* still work correctly because vendor matches a real segment and sets literalMatched = true.

This is a common pattern in real AGENTS.yaml files — exclude: ["**/*.py"], exclude: ["**/generated/**"] with only 2 segments after split — so it'll bite people quickly.


Warnings

internal/core/engine.go:388dirSlashPatternMatches misses ./**/ at root

The ./**/ pattern (meaning "this directory and all subdirectories") doesn't match when relDir="". The special-case code checks for exact ./ then strips **/ prefixes, but ./**/ starts with ./ not **/, so the stripping never fires. Confirmed with a unit test.

Minor edge case since users would probably write **/ instead, but ./ is documented as valid in docs/examples.md, so ./**/ is a natural extension. Fix is to strip the ./ prefix before the **/ stripping loop.

internal/core/engine_test.go — no test for **/*.ext exclude in directory queries

The existing exclude tests use **/vendor/** (3 segments, has a literal) and vendor/ (trailing-slash). Both pass correctly. The bug above only shows up with 2-segment **/filename patterns, which happen to be one of the most common exclude forms. Adding a test like exclude: ["**/*.py"] against a directory query would have caught this.

cmd/sctx/main.go:352isDir falls back on any os.Stat error

If Stat fails for permission denied or symlink loop (not just "not exists"), the function falls through to trailing-slash detection. A directory without a trailing slash gets misclassified as a file, producing silently wrong results. Worth at minimum a comment; ideally check os.IsNotExist before the fallback.

internal/core/engine.go:428**/-prefix short-circuit in fileGlobMatchesDir

All patterns starting with **/ match every directory. So match: ["**/api/*.py"] matches tests/, vendor/, etc. The PR description says this is intentional, and tracing through the segment walker shows it'd return true anyway (since ** can bridge to any nested api/). So this is really just an optimization that happens to produce the same result. A short comment explaining that would save the next reader some time.


Notes

internal/core/engine.go:558literalMatched naming is misleading

It tracks whether any non-** segment matched, including wildcards like * and *.py. The name suggests only fixed strings count. Something like segmentValidated would be clearer.

internal/core/engine.go:493 — recursive branching on ** segments

matchSegments/matchSegmentsStrict are O(n^k) for k non-adjacent ** segments at directory depth n. collapseDoubleStars handles adjacent ones but not a/**/b/**/c. Fine for real-world patterns (rarely >2 **), but a max-depth bail-out would be cheap insurance if you want it.


The critical exclude bug is a blocker — it'll silently drop context for common patterns. The rest is cleanup. Overall the design is solid and the test suite is unusually good.

Developer metrics

Total duration: 6m 42s
Turns: 57
Tool calls: 39
Tokens: 1,772,770 input / 15,836 output

Stage Model Duration Turns Tool calls Tokens (in/out) Cache read Cache creation
analyze claude-opus-4-6 1m 51s 27 22 632,757 / 2,725 590,485 42,257
review_findings claude-opus-4-6 4m 18s 29 16 998,924 / 11,839 938,699 60,205
draft claude-opus-4-6 0m 32s 1 1 141,089 / 1,272 78,536 62,550

@gregology
Copy link
Copy Markdown
Contributor

gregology bot commented Mar 24, 2026

Review: directory-scoped matching with trailing-slash patterns

Adds directory queries to the resolution engine via a trailing-slash convention (tests/ targets a directory, not files) and a new DirPath field on ResolveRequest. The CLI auto-detects directories. The matching logic splits into generous-match / strict-exclude paths for directory queries, with a segment-walking algorithm that handles **, *, and literal path components.

Nice piece of work — the design is well-motivated, the trailing-slash convention is the right call, and the test coverage is thorough. A few things caught my eye:


Warnings

docs/examples.md:312 — example is misleading about file query visibility

The new webhook-handlers decision uses match: ["./"] with a comment saying "scoped to this directory only." But ./ is a directory pattern, so isDirPattern returns true and matchesFileGlobs skips it entirely. This decision will never appear when querying a file like packages/payments/src/checkout.ts.

The paragraph right below still says "When an agent edits packages/payments/src/checkout.ts, it gets the shared monorepo conventions and the payments-specific context." That's no longer fully accurate — the agent won't see the webhook decision via a file query.

Fix: either add a note that match: ["./"] entries only surface in directory queries, or swap to match: ["*"] if the intent is for it to match files too.


internal/core/engine.go:193 — filterContextDir / filterDecisionsDir duplicate action/timing logic

filterContextDir is a copy-paste of filterContext with only the glob-matching call swapped. Same for the decisions pair. If a new filter field lands later, both copies need updating or they'll silently diverge.

One way to clean this up: have filterContext accept a globMatcher func(sourceDir, path string, match, exclude []string) bool parameter and pass either matchesFileGlobs or matchesDirGlobs.


internal/core/engine.go:516matchSegmentsStrict treats * as a validating segment

literalMatched gets set to true for any non-** segment, including *. So exclude: ["*/*.py"] would exclude every top-level directory in a dir query, because * matches the dir name and flips the flag. Compare with exclude: ["**/*.py"] which correctly excludes nothing.

That asymmetry between * and ** in exclude behavior might be intentional, but it's surprising — */*.py targets files, not directories. Either tighten the check to only set the flag for segments without wildcard characters, or add a test that locks in the current behavior with a comment explaining the rationale.


internal/core/engine_test.go — property test checks no-panic but no invariants

TestResolve_DirQuery_NeverPanics is good for catching crashes, but the generous/strict asymmetry in directory matching is exactly the kind of thing where property-based invariant checking pays off. One easy invariant: an entry with default match: ["**"] and no exclude should always appear in results for any directory within the project root. That alone would catch regressions in the relDir=="" special-casing.


Notes

cmd/sctx/main.go:353 — nolint comment is inaccurate

The //nolint:gosec on os.Stat(absPath) says "absPath is derived from filepath.Abs, not user-controlled taint" — but it absolutely is user-controlled (it comes from args[0]). The real justification is that os.Stat is read-only. Something like // os.Stat is read-only, safe on user-provided paths would be more honest.

internal/adapter/claude.go:82 — hook adapters don't support directory queries yet

The Claude and Pi adapters still only set FilePath. Directory-scoped entries (match: ["tests/"]) are invisible through hooks, which is the main integration path for agents. Probably fine to defer to a follow-up, but worth tracking since the PR motivation is specifically about agents needing directory-level context before opening files.

internal/core/engine.go:489literalMatched naming is misleading

It tracks any non-** match, including wildcards like * and [abc]. The docstring gets this right but the variable name doesn't. segmentValidated or nonStarStarMatched would be clearer.


Overall this is solid. The core algorithm is well-reasoned, the test matrix for dirSlashPatternMatches at root vs child is really good, and the docs updates are thorough. The examples.md issue and the * vs ** exclude behavior are the main things I'd want addressed before merging.

Developer metrics

Total duration: 4m 58s
Turns: 43
Tool calls: 30
Tokens: 1,409,306 input / 11,505 output

Stage Model Duration Turns Tool calls Tokens (in/out) Cache read Cache creation
analyze claude-opus-4-6 1m 7s 17 14 282,540 / 1,934 238,563 43,966
review_findings claude-opus-4-6 3m 22s 25 15 972,925 / 8,323 906,493 66,410
draft claude-opus-4-6 0m 28s 1 1 153,841 / 1,248 84,924 68,914

@gregology
Copy link
Copy Markdown
Contributor

gregology bot commented Mar 24, 2026

Adds directory-scoped matching to the resolution engine — trailing-slash patterns like match: ["tests/"] that target directories without leaking into subdirectories, plus a segment-walking algorithm for determining whether file-glob patterns could hit inside a queried directory. Clean split between generous match semantics and strict exclude semantics. The CLI auto-detects directories via os.Stat with a trailing-slash fallback.

Really well-structured PR. The match/exclude asymmetry is a smart design call and the test coverage on the core engine is thorough. A few things worth looking at:


Warnings

Validator doesn't know about directory patternsinternal/validator/validate.go:220

validateGlobs calls doublestar.Match(pattern, "test") to check syntax. This happens to work for trailing-slash patterns (doublestar doesn't error on them), but the validator has no awareness of the new convention. It can't catch things like a directory pattern combined with on: edit, which doesn't really make sense for directory queries.

Suggestion: strip the trailing / before passing to doublestar.Match so the base glob gets validated explicitly. Could also warn when directory patterns are paired with action filters other than all.

Recursive backtracking in segment matchersinternal/core/engine.go

matchSegments and matchSegmentsStrict backtrack over ** segments with for skip := 0; skip <= len(dir)-di; skip++. collapseDoubleStars only removes adjacent ** segments, so a pattern like **/*/**/*/** keeps all three. Deep directory paths + multiple ** segments -> O(n^k) recursive calls. Real-world patterns won't trigger this, but a weird AGENTS.yaml could.

Suggestion: either add a recursion budget (bail after ~10k calls with a safe default), or reject patterns with more than 2-3 ** segments at validation time.


Notes

Adapters don't support directory queries yetinternal/adapter/claude.go:82

Both Claude and Pi adapters only set FilePath, so directory-scoped entries are invisible through the hook pathway. This is fine for now since hooks are file-scoped, but the PR description mentions skills as a future use case that needs this.

Worth adding a short comment near the core.Resolve calls in both adapters so someone doesn't wonder if it was an oversight.

No CLI-level tests for directory routingcmd/sctx/main.go

The isDir helper has three code paths (exists on disk as dir, doesn't exist + trailing slash, doesn't exist + no trailing slash). The core engine tests are great, but there's nothing exercising the CLI's detection logic end-to-end. A table-driven test in main_test.go hitting cmdContext with a real directory, a trailing-slash non-existent path, and a file path would close the gap.

Match/exclude asymmetry could use a concrete exampledocs/context.md

The docs explain the generous-vs-strict distinction well in prose, but a side-by-side example would make it click faster. Something like: match: ["**/vendor/**"] matches src/ (generous — vendor/ could be nested inside) while exclude: ["**/vendor/**"] does NOT exclude src/ (strict — src isn't vendor).


No blockers here. The core algorithm and tests are solid — the warnings are about hardening edge cases rather than correctness issues in normal use.

Developer metrics

Total duration: 5m 24s
Turns: 43
Tool calls: 33
Tokens: 1,224,894 input / 12,787 output

Stage Model Duration Turns Tool calls Tokens (in/out) Cache read Cache creation
analyze claude-opus-4-6 1m 16s 22 19 340,427 / 2,390 294,026 46,387
review_findings claude-opus-4-6 3m 46s 20 13 735,750 / 9,511 672,060 63,676
draft claude-opus-4-6 0m 22s 1 1 148,717 / 886 82,543 66,171

@gregology gregology merged commit c762432 into main Mar 24, 2026
2 checks passed
@gregology gregology deleted the dir-scoped-matching branch March 24, 2026 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant