Add skill invocation visibility for APM-restored skills - #49866
Conversation
- Add SkillActivation type with name, status, source, and provenance - Add extractSkillActivationsFromRun: reads agent_output.json for explicit skill_invocation items, then falls back to scanning agent log files for Copilot's skill(name) pattern and structured log forms - Wire into applyRunBehavioralSignals, finalizeAndSaveRunSummary, and the cache-hit DownloadResult reconstruction - Add SkillActivations to auditAnalysisResults, ProcessedRun, RunSummary, DownloadResult, AuditData - Render skill_activations section in gh aw audit console output - Add extractSkillActivationsFromRun unit tests (agent_output, log parsing, precedence, and provenance verification) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot applies to all skills |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 3e2cdf1. |
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds skill-invocation observability to the logs and audit pipelines.
Changes:
- Introduces skill activation models and extraction.
- Persists activations through run summaries and audit data.
- Renders activations in console and JSON audit output.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/logs_skill_activations_test.go |
Tests activation extraction and provenance. |
pkg/cli/logs_run_processor.go |
Integrates activations into run processing. |
pkg/cli/logs_models.go |
Defines activation data structures. |
pkg/cli/logs_metrics.go |
Extracts activations from output and logs. |
pkg/cli/audit.go |
Integrates activations into audit analysis. |
pkg/cli/audit_report.go |
Exposes activations in audit data. |
pkg/cli/audit_report_render.go |
Renders activations in console output. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 5
- Review effort level: Balanced
| // 3. `Skill invoked: <name>` — human-readable form (future/custom engines). | ||
| var skillInvocationPatterns = []*skillPattern{ | ||
| { | ||
| re: regexp.MustCompile(`(?i)\bskill\(([A-Za-z0-9][A-Za-z0-9_.-]*)\)`), |
| if err := json.Unmarshal(itemRaw, &item); err != nil { | ||
| continue | ||
| } | ||
| if item.Type != "skill_invocation" { |
| MissingData: results.missingData, | ||
| Noops: results.noops, | ||
| MCPFailures: results.mcpFailures, | ||
| SkillActivations: results.skillActivations, |
| agentOutputPath := filepath.Join(runDir, constants.AgentOutputArtifactName) | ||
| if stat, err := os.Stat(agentOutputPath); err == nil { | ||
| if stat.IsDir() { | ||
| nested := filepath.Join(agentOutputPath, constants.AgentOutputArtifactName) |
| // Phase 2 – scan raw agent log files for engine-specific patterns. | ||
| // Always runs so that skills not covered by agent_output.json are also captured. | ||
| // Skills already found in Phase 1 are skipped to avoid duplicates. | ||
| logActivations, logErr := extractSkillActivationsFromLogFiles(runDir, run, verbose, experimentName, variant) |
There was a problem hiding this comment.
Review: Add skill invocation visibility for APM-restored skills
The implementation is well-structured and consistent with existing audit pipeline patterns. Both extraction paths (agent_output.json and log file scanning) are clearly separated with correct precedence logic. Tests cover the core merging and deduplication scenarios thoroughly.
No blocking issues found. A few minor observations:
extractSkillActivationsFromLogFilesreads entire log files into memory withos.ReadFile; for very large log files this could be expensive, but the pattern is consistent with existing code in the file.filepath.Walkin the log-file scanner returns partial activations alongside a non-nilwalkErr. The caller treats this as a non-fatal warning (correct), so data is not lost.- The
skillInvocationPatternsslice is package-level with an unexportedskillPatterntype — consistent with the rest of the file.
Overall the PR closes the observability gap clearly and safely.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 34.4 AIC · ⌖ 7.45 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design, /diagnosing-bugs, and /tdd — requesting changes on a few correctness and consistency issues.
📋 Key Themes & Highlights
Key Themes
- Phase 2 contract mismatch: PR description says Phase 2 is skipped when Phase 1 yields results; the implementation (and the function's own doc comment) says Phase 2 always runs. One of them is wrong and needs fixing before merge.
- Dead error check:
extractSkillActivationsFromRunnever returns a non-nil error, so theskillErr != nilguards in callers are unreachable — either surface errors or remove the guards. - Render inconsistency:
renderConsoleSkillActivationsuses raw string concatenation while every other render function usesconsole.Format*helpers orfmt.Sprintf. - Unguarded string literals:
Status/Sourcevalues are magic strings; typed constants would prevent drift as new sources are added. - Shared test tmpDir:
TestExtractSkillActivationsFromAgentOutputwrites to the same path across table-driven sub-cases, which will break undert.Parallel().
Positive Highlights
- ✅ Two-phase extraction with explicit source attribution is a clean, observable design
- ✅ Tests cover all three log-file patterns, deduplication, precedence, and provenance fields
- ✅ Pipeline integration (cached restore, audit pipeline, logs pipeline) is thorough and consistent
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 49.6 AIC · ⌖ 8.28 AIC · ⊞ 7.1K
Comment /matt to run again
| )) | ||
| } | ||
| logsMetricsLog.Printf("Found %d skill activation(s)", len(activations)) | ||
| return activations, nil |
There was a problem hiding this comment.
[/codebase-design] The PR description says Phase 2 is skipped when Phase 1 yields results, but the implementation always runs both phases and deduplicates by name. The doc comment on extractSkillActivationsFromRun also says "Phase 2 always runs to supplement", which contradicts the PR body. Please align the PR description or the implementation — if the contract is supplement, the code is correct but the PR body needs updating; if it is skip/fallback, the code over-reports log-parse entries alongside agent_output ones.
@copilot please address this.
| result.MCPFailures = mcpFailures | ||
|
|
||
| skillActivations, skillErr := extractSkillActivationsFromRun(runOutputDir, result.Run, verbose, expName, expVariant) | ||
| if skillErr != nil && verbose { |
There was a problem hiding this comment.
[/diagnosing-bugs] extractSkillActivationsFromRun always returns a nil error (it absorbs errors internally and logs via verbose). The skillErr != nil guard here is therefore dead code and will never fire. This is inconsistent with how other extractors in the same file are wired.
💡 Suggestion
Either surface real errors from extractSkillActivationsFromRun so callers can react, or remove the dead check and inline a comment noting that errors are already handled internally. The inconsistency will confuse the next reader.
@copilot please address this.
| if len(activations) == 0 { | ||
| return | ||
| } | ||
| fmt.Fprintln(os.Stderr, " skill_activations:") |
There was a problem hiding this comment.
[/codebase-design] renderConsoleSkillActivations builds lines with raw string concatenation (+ operator) while every other render function in this file uses a console.Format* helper or a format string. This is a style inconsistency that will make future colour/padding changes miss this section.
💡 Suggestion
Follow the pattern used by renderConsoleMissingTools and renderConsoleMCPFailures — either use fmt.Sprintf for the line or introduce a matching console.Format helper, whichever fits the output style of adjacent sections.
@copilot please address this.
| } | ||
|
|
||
| // SkillActivation records a detected skill invocation from agent logs. | ||
| // Source indicates where the invocation was detected: "agent_output" for |
There was a problem hiding this comment.
[/codebase-design] Status and Source on SkillActivation are open strings with only informal constants in comments (// "invoked", // "agent_output" or "log_parse"). All other report types in this file use the same pattern, but these two fields are the most likely to diverge as new sources are added.
💡 Suggestion
Consider introducing typed constants (e.g. SkillActivationStatusInvoked, SkillActivationSourceAgentOutput, SkillActivationSourceLogParse) in the same file or logs_skill_activations.go. This makes future callsites self-documenting and prevents silent string drift.
@copilot please address this.
| // TestExtractSkillActivationsFromLogFiles verifies that skill invocation patterns | ||
| // in raw agent log files are detected when no agent_output.json is present. | ||
| func TestExtractSkillActivationsFromLogFiles(t *testing.T) { | ||
| testRun := WorkflowRun{DatabaseID: 456, WorkflowName: "log-parse-test"} |
There was a problem hiding this comment.
[/tdd] The test suite uses a shared tmpDir across all sub-cases in TestExtractSkillActivationsFromAgentOutput and uses t.Cleanup to remove the written file, but all sub-cases write to the same path (tmpDir/agent_output.json). If two sub-cases ever run concurrently (e.g. with t.Parallel()), they will clobber each other's files.
💡 Suggestion
Move tmpDir := testutil.TempDir(t, ...) inside each sub-case's t.Run closure (like the TestExtractSkillActivationsFromLogFiles test already does) so each sub-case gets an isolated directory.
@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 95/100 — Excellent
📊 Metrics (5 tests, 14 scenarios)
Highlights
Verdict
|
Captures the architectural decision to surface SkillActivation records via dual-source extraction (agent_output.json + log-file pattern scan) as required by the design decision gate for PR #49866.
|
@copilot Please address the remaining blockers on this PR, then run the Outstanding review items (newest first):
Branch refresh may help once you are ready.
|
|
🎉 This pull request is included in a new release. Release: |
No first-class signal existed to confirm whether an APM-restored skill was actually invoked by the agent at runtime — only that it was installed. This closes that observability gap by parsing and surfacing skill invocation records through the full audit pipeline.
New type:
SkillActivationAdded to
logs_models.gowithname,status("invoked"),source("agent_output"|"log_parse"), and standardReportProvenancefields (run_id, workflow_name, timestamp, experiment/variant).Extraction:
extractSkillActivationsFromRunTwo-phase extraction in
logs_metrics.go:agent_output.jsonfor items with"type": "skill_invocation". Agents opt into this by emitting through the safe-output mechanism.skill(name)— Copilot coding agent explicit form (see Copilot agent ignores exact APM skill invocation form and still emits skill(name) #27555)[skills] invoked: name— structured log prefixSkill invoked: name— human-readable formPhase 2 deduplicates by skill name across files.
Pipeline integration
SkillActivationswired intoProcessedRun,RunSummary,DownloadResult(including cache-hit restore),AuditData, andauditAnalysisResultsapplyRunBehavioralSignals) and audit pipeline (launchCoreAuditAnalyses), with full experiment provenancerun_summary.jsongh aw auditconsole output:gh aw audit --jsonunderskill_activationsTests
pkg/cli/logs_skill_activations_test.gocovers agent_output parsing, all three log-file patterns, deduplication, agent_output-over-log_parse precedence, and provenance field population.