Skip to content

Conversation

@swedishborgie
Copy link
Collaborator

@swedishborgie swedishborgie commented Nov 12, 2025

This pull request adds the following features:

  • You'll be able to set filters on tasks such that you'll be able to include / exclude tools and rules based on labels (similar to how current CLI works with the -s flag.
  • You'll be able to optionally emit task frontmatter to the emitted prompt via the -t flag.

Example:
rule1.md

---
rule: rule1
---

rule2.md

---
rule: rule2
---

rule3.md

---
rule: rule3
---

task.md

---
task_name: do_thing
selectors:
  rule: [rule1, rule2]
---

Would select rule1, rule2.

coding-context -s rule=rule1 -s rule=rule2 do_thing

Should do the same thing.

Setting an empty key (e.g. -s rule=) will exclude all rules files with the rule key set.

Non-functionally:

  • Moves to a supported YAML library.
  • Cleans up main.go:
    • Adds tests (~70% coverage)
    • Splits up run() into several different functions for readability.

Split this into several different commits to hopefully make reviewing easier.

@swedishborgie swedishborgie force-pushed the main branch 3 times, most recently from 532e5ed to f547a7b Compare November 12, 2025 21:43
mpowers5 added 4 commits November 12, 2025 21:53
Previous library is archived and is no longer maintained.
Changes:
 * Moved static path lists to paths.go
 * Removed global state for testability.
 * Split run() into multiple functions for readability / maintainability.
 * Added tests.
For Example:
```markdown
  ---
  task_name: my_task
  selectors:
    language: go
    env: prod
  ---
```

Would be the same as:
```
-s language=go -s env=prod
```
@alexec
Copy link
Contributor

alexec commented Nov 12, 2025

I agree with the "selectors". I think they should be a map rather than a list,

selectors:
  tool_name: [ gh, jira ]

Copilot finished reviewing on behalf of alexec November 12, 2025 22:08
Copy link
Contributor

Copilot AI left a comment

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 adds support for task-level selectors to filter rules/tools, enables optional task frontmatter emission via the -t flag, and refactors main.go for improved testability by introducing a codingContext struct. The PR also migrates from gopkg.in/yaml.v3 to github.com/goccy/go-yaml for YAML parsing.

Key Changes

  • Selector enhancement: Changed selectorMap from simple string values to string slices, enabling OR logic when multiple values are specified for the same key
  • Task frontmatter selectors: Tasks can now define selectors in their frontmatter to filter which rules/tools are included
  • Improved testability: Refactored main.go by extracting a codingContext struct with injected dependencies (output writers, command runner), enabling comprehensive unit testing

Reviewed Changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
selector_map.go Changed selectorMap implementation to use string slices for OR logic support, updated matching algorithm
selector_map_test.go Added comprehensive test coverage for array selectors, OR logic, and mixed selector scenarios
main.go Refactored into codingContext struct with extracted methods; added parseTaskFile to extract selectors from task frontmatter
main_test.go Added extensive test suite (~70% coverage) with table-driven tests for all major functions
markdown.go Removed parseMarkdownFileWithRawFrontmatter function, simplified to single parseMarkdownFile function; switched to goccy/go-yaml
remote.go Extracted downloadRemoteDirectories and cleanupDownloadedDirectories methods into codingContext
paths.go New file extracting path definitions into reusable functions (allTaskSearchPaths, allRulePaths, etc.)
go.mod / go.sum Migrated from gopkg.in/yaml.v3 to github.com/goccy/go-yaml v1.18.0

@swedishborgie
Copy link
Collaborator Author

@alexec -- Think this is ready for a final review. I've addressed all the copilot feedback and I've manually tested. This PR should be entirely backwards compatible.

@alexec alexec merged commit 3837ba6 into kitproj:main Nov 13, 2025
5 checks passed
Copilot finished reviewing on behalf of alexec November 13, 2025 16:17
Copy link
Contributor

Copilot AI left a comment

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 9 out of 10 changed files in this pull request and generated 2 comments.

Comment on lines 86 to 101
func (includes *selectorMap) matchesIncludes(frontmatter frontMatter) bool {
for key, value := range *includes {
for key, values := range *includes {
fmValue, exists := frontmatter[key]
// If key exists, it must match the value
if exists && fmt.Sprint(fmValue) != value {
if !exists {
// If key doesn't exist in frontmatter, allow it
continue
}

// Check if frontmatter value matches any element in the inner map (OR logic)
fmStr := fmt.Sprint(fmValue)
if !values[fmStr] {
return false
}
// If key doesn't exist, allow it
}
return true
}
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

[nitpick] The matchesIncludes function doesn't correctly handle empty value selectors (e.g., -s env=). According to the test cases, when a selector has an empty value and the key exists in frontmatter, it should not match. However, the current implementation doesn't check for this case.

When Set("env=") is called, it creates an empty map for the "env" key (line 47). Then in matchesIncludes, when the key exists in frontmatter (line 89), it converts the frontmatter value to string (line 95) and checks if it exists in the empty map (line 96). Since the map is empty, values[fmStr] will be false, which correctly returns false.

However, there's a logical inconsistency: an empty map map[string]bool{} means "match nothing", but the test expects it to mean "exclude files with this key". The current implementation happens to work due to the empty map behavior, but this is not semantically clear and could be confusing.

Copilot uses AI. Check for mistakes.
case []any:
// Convert []any to map[string]bool
for _, item := range v {
cc.includes.SetValue(key, fmt.Sprint(item))
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

[nitpick] The type conversion for []interface{} doesn't handle all possible YAML array element types. While fmt.Sprint(item) works for most cases, it may produce unexpected string representations for complex types (e.g., nested maps/arrays).

Consider adding explicit type checking for array elements or documenting that only scalar values are supported in selector arrays.

Suggested change
cc.includes.SetValue(key, fmt.Sprint(item))
switch itemTyped := item.(type) {
case string:
cc.includes.SetValue(key, itemTyped)
case int, int8, int16, int32, int64, uint, uint8, uint16, uint32, uint64, float32, float64, bool:
cc.includes.SetValue(key, fmt.Sprint(itemTyped))
default:
return fmt.Errorf("task file %s has invalid selector array value for key %q: expected scalar, got %T", cc.matchingTaskFile, key, item)
}

Copilot uses AI. Check for mistakes.
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.

2 participants