Skip to content

Fix glob resolution in catalog directives within included files - #134

Merged
jeduden merged 7 commits into
mainfrom
claude/fix-issue-133-10VOU
Apr 19, 2026
Merged

Fix glob resolution in catalog directives within included files#134
jeduden merged 7 commits into
mainfrom
claude/fix-issue-133-10VOU

Conversation

@jeduden

@jeduden jeduden commented Apr 11, 2026

Copy link
Copy Markdown
Owner

Summary

This PR fixes glob resolution for catalog directives that are transplanted into documents via file inclusion. When a catalog directive is included from a subdirectory, its glob patterns now correctly resolve relative to that subdirectory rather than the including file's directory.

Key Changes

  • Include rule enhancement: Added injectSourceDir() function that automatically injects a source-dir parameter into multi-line processing instructions when content is included from a different directory. This ensures downstream directives (like catalog) know where to resolve globs from.

  • Catalog rule updates:

    • Added resolveGlobFS() to determine the correct filesystem and path prefix for glob resolution based on the source-dir parameter
    • Refactored resolveGlobMatches() into resolveGlobMatchesFrom() to accept an explicit filesystem for glob resolution
    • Updated buildCatalogEntries() to use the source-dir-aware filesystem and prefix matched filenames so links resolve correctly from the including file
    • Fixed resolveGitignore() to compute the gitignore base directory relative to source-dir when present
  • Test coverage: Added comprehensive tests for:

    • Glob resolution from subdirectories with source-dir
    • Front matter reading from the correct directory
    • Exclude patterns working relative to source-dir
    • Source-dir injection into included catalog directives
    • Preservation of existing parameters during injection
    • Avoiding unnecessary injection when files are in the same directory

Implementation Details

The solution works by having the include rule detect when content is being included from a different directory and automatically inject a source-dir parameter into multi-line processing instructions. This parameter is then used by the catalog rule to:

  1. Resolve globs against the correct subdirectory of the root filesystem
  2. Prefix matched filenames so they resolve correctly from the including file's location
  3. Read front matter from files in the correct directory

The injection is skipped for single-line PIs and when content is wrapped in code fences (where PIs aren't parsed).

https://claude.ai/code/session_01VN3XGWEbs4qRPet8qBia59

Copilot AI review requested due to automatic review settings April 11, 2026 10:00
@codecov

codecov Bot commented Apr 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.30769% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.31%. Comparing base (8377db3) to head (8d714ba).
⚠️ Report is 38 commits behind head on main.

Files with missing lines Patch % Lines
internal/rules/catalog/rule.go 89.18% 3 Missing and 1 partial ⚠️
internal/rules/include/rule.go 95.12% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #134      +/-   ##
==========================================
+ Coverage   86.25%   86.31%   +0.05%     
==========================================
  Files          92       92              
  Lines        9880     9950      +70     
==========================================
+ Hits         8522     8588      +66     
- Misses        886      889       +3     
- Partials      472      473       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aims to fix <?catalog?> glob resolution when a catalog directive is brought into another document via <?include?>, by carrying the included file’s directory context forward (via a source-dir parameter) and making the catalog rule resolve globs/front matter relative to that directory.

Changes:

  • Include expansion: injects a source-dir parameter into multi-line processing instructions in included content when inclusion crosses directories.
  • Catalog rule: resolves globs and reads front matter from a source-dir-scoped filesystem; prefixes generated filenames so links work from the including file.
  • Tests: adds coverage for source-dir injection and catalog glob/front matter/exclude behavior under source-dir.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
internal/rules/include/rule.go Injects source-dir into included processing instructions to preserve directory context for downstream directives.
internal/rules/include/rule_test.go Adds tests validating source-dir injection behavior during include fix.
internal/rules/catalog/rule.go Introduces source-dir-aware glob FS selection and uses it for globbing/front matter, with path prefixing for output filenames.
internal/rules/catalog/rule_test.go Adds tests ensuring catalog globbing/front matter/excludes work correctly when source-dir is provided.

Comment thread internal/rules/include/rule.go Outdated
Comment thread internal/rules/catalog/rule.go Outdated
Comment thread internal/rules/catalog/rule.go
Copilot AI review requested due to automatic review settings April 11, 2026 11:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comment thread internal/rules/include/rule.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

.github/copilot-instructions.md:27

  • Adding source-dir: "." changes {filename} to be emitted relative to this file’s directory (it will be prefixed with .. for .github/). With the current row: "- [{summary}](../{filename})", generated links will likely become ../../docs/... (double ..) instead of the intended ../docs/.... Consider updating the row template to link directly to {filename} (or otherwise adjust) now that filenames are source-dir-aware.
<?catalog
source-dir: "."
glob:
  - "docs/**/*.md"
  - "!docs/research/**"
  - "!docs/security/**"
sort: path
header: ""
row: "- [{summary}](../{filename})"
?>

Comment thread internal/rules/catalog/rule.go

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

internal/rules/catalog/rule.go:312

  • buildCatalogEntries now stores the display path (with prefix, which can be ".." when source-dir: "." is used from a subdirectory) into the filename field. Downstream logic like checkCatalogIncludeCycle reads files via f.FS using this filename value; paths containing .. are invalid for os.DirFS, so cycle detection will silently stop working for those entries.

Consider keeping a separate internal path for filesystem access (e.g., repo-root-relative path when source-dir is set, or the raw glob match) and update the cycle check to use that, while continuing to use the prefixed display path for links/templates.

	for _, p := range files {
		displayPath := p
		if prefix != "" {
			displayPath = path.Join(prefix, p)
		}
		fields := map[string]any{"filename": displayPath}
		if needFM {
			for k, v := range readFrontMatter(globFS, p) {
				fields[k] = v
			}
		}
		entries = append(entries, fileEntry{fields: fields})

@jeduden jeduden added queue Add to a PR to enqueue it queue:active Applied automatically when a PR is in an active batch and removed queue Add to a PR to enqueue it labels Apr 17, 2026
jeduden added a commit that referenced this pull request Apr 17, 2026
@jeduden jeduden added queue:failed Applied automatically when CI fails or merge conflict occurs and removed queue:active Applied automatically when a PR is in an active batch labels Apr 17, 2026
@jeduden

jeduden commented Apr 17, 2026

Copy link
Copy Markdown
Owner Author

Merge queue: CI failed

claude added 7 commits April 17, 2026 20:26
When a file containing a <?catalog?> directive is included via
<?include?>, the catalog's glob pattern was resolving relative to
the including file's directory instead of the included file's
directory.

Fix: the include rule now injects a source-dir parameter into
processing instruction YAML bodies during expansion. The catalog
rule reads source-dir and resolves globs from that subdirectory
of RootFS, prefixing matched filenames so relative links work
from the including file.

https://claude.ai/code/session_01VN3XGWEbs4qRPet8qBia59
- Remove includedDir != "." guard so root-level includes get
  source-dir injection (review comment #1)
- Compute display prefix relative to the catalog-owning file's
  directory via filepath.Rel, so links are correct when the
  includer is in a subdirectory (review comments #2, #3)
- Handle source-dir: "." by using RootFS directly
- Remove unused resolveGlobMatches wrapper (lint fix)
- Add tests for root-include and sibling-subdir edge cases
- Update copilot-instructions.md (mdsmith fix)

https://claude.ai/code/session_01VN3XGWEbs4qRPet8qBia59
Add tests for uncovered branches in resolveGlobFS and
injectSourceDir:

- source-dir: "." with file at root (relPrefix == ".")
- source-dir matching file's own directory (no prefix)
- source-dir without RootFS (fallback to f.FS)
- single-line PIs skipped by injection
- PIs that already have source-dir (no double injection)
- plain text content with no PIs

resolveGlobFS: 78.9% → 89.5%
injectSourceDir: 85.3% → 97.1%

https://claude.ai/code/session_01VN3XGWEbs4qRPet8qBia59
- Add test for filepath.Rel failure (absolute f.Path, relative
  source-dir)
- Fix fs.Sub error test: use absolute paths for both f.Path and
  source-dir so Rel succeeds but fs.Sub fails
- resolveGlobFS: 89.5% → 100%
- injectSourceDir: 97.1% (remaining line is unreachable — goldmark
  parser never errors)

https://claude.ai/code/session_01VN3XGWEbs4qRPet8qBia59
Escape the source-dir value with strconv.Quote so directory
names containing quotes or backslashes produce valid YAML.

https://claude.ai/code/session_01VN3XGWEbs4qRPet8qBia59
Reject source-dir values containing ".." or absolute paths when
computing the gitignore base directory, preventing traversal
outside the project root. Mirrors the validation already done
by fs.Sub in resolveGlobFS.

https://claude.ai/code/session_01VN3XGWEbs4qRPet8qBia59
Update TestFix_RecursiveExpansionSubdir to expect source-dir
injection in nested include PIs from subdirectories.
Regenerate copilot-instructions.md after CLAUDE.md changes on main.

https://claude.ai/code/session_01VN3XGWEbs4qRPet8qBia59
@jeduden
jeduden force-pushed the claude/fix-issue-133-10VOU branch from 9821310 to 8d714ba Compare April 17, 2026 20:29
@jeduden jeduden added queue Add to a PR to enqueue it and removed queue:failed Applied automatically when CI fails or merge conflict occurs labels Apr 19, 2026
@jeduden jeduden added queue:active Applied automatically when a PR is in an active batch and removed queue Add to a PR to enqueue it labels Apr 19, 2026
@jeduden

jeduden commented Apr 19, 2026

Copy link
Copy Markdown
Owner Author

🟢 Merge Queue — picked up

This PR is in the queue and will be batched with other queue-labelled PRs.

Next: No action needed — you'll get another comment when CI starts on the batch. View merge queue run.

@jeduden

jeduden commented Apr 19, 2026

Copy link
Copy Markdown
Owner Author

🔵 Merge Queue — CI running

Merged into batch branch merge-queue/batch-134-1776608153. View CI run.

Next: No action needed — you'll be notified when CI completes.

@jeduden

jeduden commented Apr 19, 2026

Copy link
Copy Markdown
Owner Author

Merge Queue — merged

This PR landed on main via commit 1bde75e. CI run that validated the merge.

Next: Done — nothing more to do here.

@jeduden jeduden removed the queue:active Applied automatically when a PR is in an active batch label Apr 19, 2026
@jeduden
jeduden merged commit 1bde75e into main Apr 19, 2026
12 checks passed
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.

3 participants