Conversation
Co-authored-by: alexec <1142830+alexec@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Removes URN parsing/support from markdown frontmatter to drop the github.com/leodido/go-urn dependency and rely on existing identifiers (e.g., filename-derived Name / inline Content) instead.
Changes:
- Removed
URNfield andid→URN alias parsing fromBaseFrontMatterYAML unmarshalling. - Dropped
github.com/leodido/go-urnfromgo.mod/go.sumand removed URN helper test file. - Updated tests to stop asserting URN behavior and to treat
idas an inlineContentkey.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/codingcontext/markdown/frontmatter.go | Removes URN fields/parsing; keeps custom UnmarshalYAML to reliably populate inline Content. |
| go.mod | Drops github.com/leodido/go-urn requirement. |
| go.sum | Removes go-urn checksums. |
| pkg/codingcontext/markdown/urn_test.go | Deletes URN test helpers. |
| pkg/codingcontext/markdown/frontmatter_task_test.go | Removes URN expectations; updates unmarshal expected Content to include id. |
| pkg/codingcontext/markdown/frontmatter_rule_test.go | Removes URN expectations; updates unmarshal expected Content to include id. |
| pkg/codingcontext/markdown/frontmatter_command_test.go | Removes URN expectations; updates unmarshal expected Content to include id. |
| pkg/codingcontext/result_test.go | Removes URN usage in MCP server tests (now constructs rules without URNs). |
| pkg/codingcontext/context_test.go | Removes URN-related integration assertions around tasks/rules. |
Comments suppressed due to low confidence (2)
pkg/codingcontext/markdown/frontmatter_rule_test.go:162
- This test now sets
want.BaseFrontMatter.Content(includingid), but the assertions never comparegot.Contenttowant.Content. Add a check for the captured inline fields (at leastid) so the new behavior is covered.
// Compare fields individually
if got.Name != tt.want.Name {
t.Errorf("Name = %q, want %q", got.Name, tt.want.Name)
}
if got.Description != tt.want.Description {
t.Errorf("Description = %q, want %q", got.Description, tt.want.Description)
}
if got.Agent != tt.want.Agent {
t.Errorf("Agent = %q, want %q", got.Agent, tt.want.Agent)
}
pkg/codingcontext/markdown/frontmatter_command_test.go:160
- The test cases populate
want.BaseFrontMatter.Contentwithid, but the assertions only compare typed fields and never verifygot.Content. Add an assertion forgot.Content["id"](or compare the fullContentmap) to ensure the inline capture behavior is covered.
// Compare fields individually
if got.Name != tt.want.Name {
t.Errorf("Name = %q, want %q", got.Name, tt.want.Name)
}
if got.Description != tt.want.Description {
t.Errorf("Description = %q, want %q", got.Description, tt.want.Description)
}
if tt.want.ExpandParams != nil {
if (got.ExpandParams == nil) != (tt.want.ExpandParams == nil) {
t.Errorf("ExpandParams nil mismatch: got %v, want %v", got.ExpandParams == nil, tt.want.ExpandParams == nil)
} else if got.ExpandParams != nil && tt.want.ExpandParams != nil {
if *got.ExpandParams != *tt.want.ExpandParams {
t.Errorf("ExpandParams = %v, want %v", *got.ExpandParams, *tt.want.ExpandParams)
}
}
}
})
| // Compare fields individually for better error messages | ||
| gotTaskName, _ := got.Content["task_name"].(string) | ||
| wantTaskName, _ := tt.want.Content["task_name"].(string) | ||
| if gotTaskName != wantTaskName { | ||
| t.Errorf("TaskName = %q, want %q", gotTaskName, wantTaskName) | ||
| } |
There was a problem hiding this comment.
The updated expectations add id into BaseFrontMatter.Content, but the test assertions only check task_name and typed fields. Add an assertion for got.Content["id"] (or compare the full Content map) so the behavior “id is captured as inline content” is actually validated.
| { | ||
| FrontMatter: markdown.RuleFrontMatter{ | ||
| BaseFrontMatter: markdown.BaseFrontMatter{URN: mustParseURN("urn:agents:rule:jira-server")}, | ||
| MCPServer: mcp.MCPServerConfig{Type: mcp.TransportTypeStdio, Command: "jira"}, | ||
| MCPServer: mcp.MCPServerConfig{Type: mcp.TransportTypeStdio, Command: "jira"}, | ||
| }, | ||
| }, | ||
| { | ||
| FrontMatter: markdown.RuleFrontMatter{ | ||
| BaseFrontMatter: markdown.BaseFrontMatter{URN: mustParseURN("urn:agents:rule:api-server")}, | ||
| MCPServer: mcp.MCPServerConfig{Type: mcp.TransportTypeHTTP, URL: "https://api.example.com"}, | ||
| MCPServer: mcp.MCPServerConfig{Type: mcp.TransportTypeHTTP, URL: "https://api.example.com"}, | ||
| }, |
There was a problem hiding this comment.
These rules don’t set FrontMatter.Name, but Result.MCPServers() uses rule.FrontMatter.Name as the map key. With empty names, later entries overwrite earlier ones and the test won’t catch regressions around handling multiple servers; set distinct rule names here (or mirror Context’s filename->Name defaulting) and assert all expected entries are present.
| { | ||
| FrontMatter: markdown.RuleFrontMatter{ | ||
| BaseFrontMatter: markdown.BaseFrontMatter{URN: mustParseURN("urn:agents:rule:server1")}, | ||
| MCPServer: mcp.MCPServerConfig{Type: mcp.TransportTypeStdio, Command: "server1"}, | ||
| MCPServer: mcp.MCPServerConfig{Type: mcp.TransportTypeStdio, Command: "server1"}, | ||
| }, | ||
| }, | ||
| { | ||
| FrontMatter: markdown.RuleFrontMatter{ | ||
| BaseFrontMatter: markdown.BaseFrontMatter{URN: mustParseURN("urn:agents:rule:server2")}, | ||
| MCPServer: mcp.MCPServerConfig{Type: mcp.TransportTypeStdio, Command: "server2"}, | ||
| MCPServer: mcp.MCPServerConfig{Type: mcp.TransportTypeStdio, Command: "server2"}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
These rules are constructed without FrontMatter.Name, which causes key collisions in Result.MCPServers() (it indexes by FrontMatter.Name). Give each rule a distinct Name (or simulate Context’s defaulting) so this test actually validates behavior with multiple MCP servers instead of exercising overwrite-by-empty-key.
| FrontMatter: markdown.RuleFrontMatter{ | ||
| BaseFrontMatter: markdown.BaseFrontMatter{URN: mustParseURN("urn:agents:rule:explicit")}, | ||
| MCPServer: mcp.MCPServerConfig{Type: mcp.TransportTypeStdio, Command: "server1"}, | ||
| MCPServer: mcp.MCPServerConfig{Type: mcp.TransportTypeStdio, Command: "server1"}, | ||
| }, | ||
| }, | ||
| { | ||
| FrontMatter: markdown.RuleFrontMatter{ | ||
| BaseFrontMatter: markdown.BaseFrontMatter{URN: mustParseURN("urn:agents:rule:some-rule")}, | ||
| MCPServer: mcp.MCPServerConfig{Type: mcp.TransportTypeStdio, Command: "server2"}, | ||
| MCPServer: mcp.MCPServerConfig{Type: mcp.TransportTypeStdio, Command: "server2"}, | ||
| }, | ||
| }, | ||
| { | ||
| FrontMatter: markdown.RuleFrontMatter{ | ||
| BaseFrontMatter: markdown.BaseFrontMatter{URN: mustParseURN("urn:agents:rule:another")}, | ||
| MCPServer: mcp.MCPServerConfig{Type: mcp.TransportTypeHTTP, URL: "https://example.com"}, | ||
| MCPServer: mcp.MCPServerConfig{Type: mcp.TransportTypeHTTP, URL: "https://example.com"}, | ||
| }, |
There was a problem hiding this comment.
All rules in this case omit FrontMatter.Name, but Result.MCPServers() uses Name as the map key; this makes the last MCP server silently win. Set distinct names on each RuleFrontMatter (or mirror the filename defaulting) and update the expected map so the test covers multiple returned servers.
The
BaseFrontMatterstruct carried aURN *urn.URNfield (withidas a YAML alias) that added thegithub.com/leodido/go-urndependency but provided no functional value — file path is sufficient as an identifier.Changes
frontmatter.go: RemoveURNfield fromBaseFrontMatterandbaseFrontMatterRaw; removeid→URN alias parsing fromUnmarshalYAML. The method itself is retained (simplified) to work around a yaml.v3 limitation with doubly-inlined structs not populating inlinemap[string]anyfields.go.mod/go.sum: Dropgithub.com/leodido/go-urndependency.urn_test.gohelper file and all URN-related test cases/assertions acrossfrontmatter_*_test.go,result_test.go, andcontext_test.go. Files withid: urn:...in frontmatter continue to parse cleanly —idis now captured as a regular inline content key.✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.