Repository navigation
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ponytail pass complete: one meaningful simplification opportunity found in tool-usage merging logic; everything else looks appropriately scoped.
net: -12 lines possible.
Generated by ✂️ Ponytail Reviewer for #63688 · codex · gpt53codex · 4.4 AIC · ⌖ 2.76 AIC · ⊞ 12.5K
Comment /ponytail to run again
Comments that could not be inline-anchored
pkg/cli/audit_report.go:64
pkg/cli/audit_report.go:L64: shrink: two-pass merge/delete for skill counts in buildAuditToolUsage. append per activation directly to usage map/slice in one pass, then materialize once.
🏗️ ADR requiredAn ADR was required for this PR because it adds 169 lines in default business-logic directories, exceeding the 100-line enforcement threshold. Evidence reviewed
Gate result
Next action
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Fresh audit paths lose compact skill aggregates, while no-ID failures and cross-file timestamps are recorded incorrectly.
Review effort: Balanced
Findings: 4
Open (4)
What changed in this PR
Adds compact skill-usage aggregation and exposes invocation metrics in audit output.
Changes:
- Aggregates named skill invocations, failures, and timestamps.
- Backfills audit activations and reports
skill:<name>tool usage. - Extends schemas, cache preservation, and tests.
| File | Description |
|---|---|
actions/setup/js/generate_usage_activity_summary.cjs |
Aggregates skill activity from session events. |
actions/setup/js/generate_usage_activity_summary.test.cjs |
Tests skill aggregation. |
pkg/cli/logs_usage_activity.go |
Loads and applies compact skill aggregates. |
pkg/cli/logs_usage_activity_test.go |
Tests skill backfilling. |
pkg/cli/logs_models.go |
Adds invocation and failure counts. |
pkg/cli/audit_report.go |
Adds named skills to tool usage. |
pkg/cli/audit_run_pipeline.go |
Preserves cached skill activations. |
pkg/cli/audit_summary_build_test.go |
Tests audit integration. |
schemas/audit.schema.json |
Extends audit activation schema. |
schemas/logs-jsonl.schema.json |
Extends cached JSONL schema. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| aggregate.invocation_count += 1; | ||
| aggregate.last_timestamp = String(entry.timestamp || aggregate.last_timestamp); |
| if (data.toolCallId) { | ||
| pendingSkills.set(String(data.toolCallId), skillName); | ||
| } |
| errors := extractAuditErrors(run) | ||
| downloadedFiles := extractDownloadedFiles(run.LogsPath) | ||
| toolUsage := buildAuditToolUsage(metrics, mcpToolUsage) | ||
| toolUsage := buildAuditToolUsage(metrics, mcpToolUsage, processedRun.SkillActivations) |
| MissingData: summary.MissingData, | ||
| Noops: summary.Noops, | ||
| MCPFailures: summary.MCPFailures, | ||
| SkillActivations: summary.SkillActivations, |
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Verdict
Non-blocking overall, but the new skill-usage aggregation now records order-dependent provenance timestamps.
Theme
first_timestamp/last_timestampare taken from directory traversal order instead of min/max event time, so the audit summary can report the wrong window for a skill invocation.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 61 AIC · ⌖ 7.07 AIC · ⊞ 20.2K
Comment /review to run again
| last_timestamp: String(entry.timestamp || ""), | ||
| }; | ||
| aggregate.invocation_count += 1; | ||
| aggregate.last_timestamp = String(entry.timestamp || aggregate.last_timestamp); |
There was a problem hiding this comment.
These timestamps are derived from filesystem iteration order rather than event time, so first_timestamp/last_timestamp can be wrong as soon as the run has multiple events.jsonl files or readdir returns a different order; that makes the new audit artifact lie about when a skill first or last ran.
💡 Why this matters and how to fix it
findFiles() just walks directories in readdirSync() order, and this code initializes first_timestamp from the first file it happens to see while blindly overwriting last_timestamp on every later match. That means a later-processed older event can move last_timestamp backwards, and an earlier event discovered late can never fix first_timestamp.
Please compare timestamps and keep the minimum as first_timestamp and the maximum as last_timestamp instead of relying on traversal order.
|
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes for a test-coverage gap in new logic.
📋 Key Themes & Highlights
Key Themes
- Test coverage gap: the "create new
SkillActivationfrom usage summary" branch inapplyUsageActivitySkills(pkg/cli/logs_usage_activity.go) is untested — only the enrichment-of-existing-activation path has a test. - Fragile assumption: the JS aggregator hard-codes
input.skillas the only recognized argument key for theskilltool, with no fallback or documented contract. - Docs drift:
docs/src/content/docs/reference/audit.mdstill describesskill_activationswithout the newinvocation_count/failed_countfields orusage_summarysource. - Minor:
backfillUsageActivityMCPMetricswas refactored from pointer-based to copy-and-reassign loops forusage.Summary/usage.Servers. This looks unrelated to skill tracking — worth double-checking it wasn't an accidental side-effect of the diff, though it appears behavior-preserving.
Positive Highlights
- ✅ Clean, minimal aggregation logic in
generate_usage_activity_summary.cjs(skills/pendingSkillsmaps keyed correctly ontoolCallId). - ✅ Good use of
omitemptyand schema updates (audit.schema.json,logs-jsonl.schema.json) kept in sync with the new fields. - ✅ Solid existing test for the enrichment path and for the JS-side aggregation (
generate_usage_activity_summary.test.cjs).
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 181.3 AIC · ⌖ 14.6 AIC · ⊞ 10K
Comment /matt to run again
| } | ||
| enriched = append(enriched, activation) | ||
| } | ||
| for _, skill := range skills.Items { |
There was a problem hiding this comment.
[/tdd] The "new skill activation" branch (lines 250-268, creating a SkillActivation for a skill not already in result.SkillActivations) has no test coverage. TestApplyUsageActivitySkillsEnrichesDetailedActivations only exercises the enrichment-of-existing-activation path.
💡 Suggested test
func TestApplyUsageActivitySkillsAddsNewActivation(t *testing.T) {
result := DownloadResult{}
summary := &usageActivitySummary{
Skills: &usageActivitySkills{
Items: []usageActivitySkillItem{{Name: "new-skill", InvocationCount: 3, FailedCount: 1, FirstTimestamp: "2026-01-01T00:00:00Z"}},
},
}
applyUsageActivitySummaryToResult(summary, &result, true)
require.Len(t, result.SkillActivations, 1)
assert.Equal(t, "usage_summary", result.SkillActivations[0].Source)
}Without this, a regression that silently drops usage-only skills (e.g. a stray continue/delete ordering bug) would go unnoticed.
@copilot please address this.
| .toLowerCase() === "skill" | ||
| ) { | ||
| const input = data.input && typeof data.input === "object" ? data.input : data.arguments && typeof data.arguments === "object" ? data.arguments : {}; | ||
| const skillName = String(input.skill || "").trim(); |
There was a problem hiding this comment.
[/codebase-design] input.skill hard-codes the assumption that the built-in skill tool's argument key is literally named skill. If this ever changes in the Copilot CLI (or differs across engine versions), this silently stops aggregating without any error signal.
💡 Suggested hardening
Consider falling back to a couple of plausible keys (similar to how extractStructuredToolInput in tool_call_details.cjs tries input/arguments/parameters/args), e.g. input.skill || input.name || input.skillName, and/or add a code comment pinning the exact tool-call shape this depends on (with a link to where the Copilot CLI's skill tool emits { skill: <name> }) so future changes to the tool schema are easier to catch.
@copilot please address this.
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Skill invocations were only available in full agent traces, preventing lightweight audit analysis. This change aggregates named skill usage into compact usage artifacts and exposes it through audit output.
Usage collection
skilltool calls in Copilot session events.Audit integration
skill:<name>tools.Schema
invocation_countandfailed_countto audit and JSONL skill activation records.{ "skills": { "total_invocations": 2, "unique_skills": 1, "items": [{ "name": "documentation", "invocation_count": 2, "failed_count": 0 }] } }Branch refresh requested by pr-sous-chef run https://github.com/github/gh-aw/actions/runs/36277822788