Repository navigation
Diagnose unavailable models and add code-specific debugging playbook - #69467
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add same-run reflected model inventory analysis, diagnostic playbook coverage, consolidated model guidance, and runtime/audit playbook links. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
pelikhan
changed the title
Use current audit finding routes, bounded cached evidence collection, and consolidated model-availability guidance. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Deduplicate model routing, shorten diagnostic lookups, require explicit investigation inputs, and bound playbook size without removing code coverage or safety constraints. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Include debug-playbook.md in the offline fallback list to match the new playbook and generated skill index. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Include the optional playbook_url in audit, grouped audit and cached logs JSONL output schemas. Exercise populated playbook links in each affected command output shape. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Great work @pelikhan 👋 — the audit model-availability diagnostics and playbook consolidation look well tested and ready for review. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
🔍 Design Decision Gate 🏗️ is checking for design decision records on this pull request... |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (553 new lines in 📄 Draft ADR generated:
📄 Draft ADR content (
|
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The new model-availability audit path has a real correctness bug: provider selection stores a pointer to the range variable, so multi-provider reflections can be analyzed against the wrong endpoint.
Blocking theme
parseModelAvailabilityInventorycan end up reading models and routing metadata from the wrong provider when the match is not the last endpoint ininventory.Endpoints.- That makes the new
model_unavailable/model_availability_unknownfindings unreliable, which is too risky for a debugging feature whose whole job is root-cause accuracy.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 93.3 AIC · ⌖ 5.78 AIC · ⊞ 12.4K
Comment /review to run again
| if endpoint != nil { | ||
| return nil, nil, "Multiple configured endpoints match the provider" | ||
| } | ||
| endpoint = &candidate |
There was a problem hiding this comment.
Taking &candidate here captures the range variable, so if the matching provider is followed by any other endpoint this pointer ends up referring to the wrong entry and the audit can report model availability from the wrong provider.
💡 Why this is blocking
inventory.Endpoints usually contains several providers. In Go, for _, candidate := range ... reuses the same variable on each iteration, so storing &candidate does not keep a stable pointer to the matched slice element. A shape like [copilot(match), openai] will leave endpoint pointing at the final openai copy, which can flip a real model_unavailable into model_availability_unknown (or vice versa) and corrupt the candidate list.
Please take the address of the slice element instead, e.g. candidate := &inventory.Endpoints[i] / endpoint = candidate, and add a regression test where the matching configured provider is followed by an unrelated endpoint.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd - this is a well-structured addition with deep modules (narrow public surface on audit_model_availability.go, rich internal validation), consistent reuse of existing helpers (normalizeModelProvider, modelNameWithoutProvider), symlink-safe file reads matching existing codebase conventions, and strong table-driven test coverage (15+ scenarios). Two minor, non-blocking suggestions posted inline.
📋 Key Themes & Highlights
Key Themes
- Magic number: the same-family candidate cap of 5 in modelAvailabilityCandidates lacks a named constant (pkg/cli/audit_model_availability.go:152).
- Heuristic coupling: the new playbook-link trigger in detect_agent_errors.cjs reuses buildOutputLines wholesale, which includes soft/already-handled flags like missing_model_pricing_error; the new tests don't pin down behavior for that ambiguous overlap.
Positive Highlights
- ✅ generateModelAvailabilityFindings correctly scopes evidence to the failing run only (verified by TestAuditModelAvailabilityDoesNotUseOtherRunEvidence) and rejects symlinked artifact paths.
- ✅ Findings carefully distinguish confirmed absence (model_unavailable) from inconclusive evidence (model_availability_unknown), avoiding false claims of model retirement.
- ✅ Consolidated debug-playbook.md is backed by its own code-coverage contract tests (test_guidance.py) that scan source registries for undocumented diagnostic codes - a nice guard against documentation drift.
- ✅ Schema (audit.schema.json, logs-jsonl.schema.json) and cache-version bump correctly updated alongside the new playbook_url field.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 98.3 AIC · ⌖ 16 AIC · ⊞ 10.1K
Comment /matt to run again
| } | ||
| } | ||
| candidates = append(candidates, candidate) | ||
| if len(candidates) == 5 { |
There was a problem hiding this comment.
[/codebase-design] The candidate cap 5 is a bare magic number with no named constant or doc comment explaining the choice.
💡 Suggestion
Extract const maxModelAvailabilityCandidates = 5 near the top of the file (alongside the regex vars) so the bound is self-documenting and easy to tune/reference from tests, consistent with how maxBytes parameters are already named explicitly elsewhere in this file.
@copilot please address this.
|
|
||
| persistAgentExecution(logContent, results, rootDir); | ||
|
|
||
| if (buildOutputLines(results).some(line => line.endsWith("=true")) || process.env.GH_AW_AGENTIC_EXECUTION_OUTCOME === "failure") { |
There was a problem hiding this comment.
[/tdd] This condition reuses buildOutputLines, which includes missing_model_pricing_error=true - a soft/already-handled case that prints its own informational message at line 320, not necessarily a failure worth a playbook link.
� Why this matters
buildOutputLines is also used to build GITHUB_OUTPUT values (line 123) and categories (line 235), so coupling the playbook-link heuristic to it means any future addition to that output list silently changes when the link is printed, without a dedicated test asserting the full truth table. The new test (detect_agent_errors.test.cjs) only covers 4 cases (empty/success, denial/success, empty/failure, model-not-supported/failure) - it doesn't test a scenario where only missing_model_pricing_error or shell_expansion_guard_rejected is true with a successful outcome, which is exactly the ambiguous case this coupling creates.
Consider either: (a) defining an explicit allowlist of hard-failure flags for the link trigger, or (b) adding a test case that pins down the expected behavior for soft/warning-only flags like missing_model_pricing_error with outcome success.
@copilot please address this.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved correctness issues can produce misleading or incomplete audit diagnostics.
4 open issues
What changed in this PR
Extends gh-aw’s run diagnostics with model-availability analysis and centralized troubleshooting guidance.
Changes:
- Diagnose Pi model rejection using same-run provider inventories.
- Link audit and runtime diagnostics to debugging playbooks.
- Update schemas, cache versioning, regression coverage, and release metadata.
| File | Description |
|---|---|
schemas/logs-jsonl.schema.json |
Allow finding playbook URLs. |
schemas/audit.schema.json |
Extend audit finding schemas. |
pkg/cli/json_schema_command_test.go |
Cover playbook URLs in schema fixtures. |
pkg/cli/data/agentic_workflows_fallback_aw_files.json |
Include the consolidated playbook. |
pkg/cli/audit_report.go |
Add the finding URL field and close-error logging. |
pkg/cli/audit_report_render.go |
Print finding playbook links. |
pkg/cli/audit_report_analysis.go |
Integrate model findings and recommendations. |
pkg/cli/audit_model_availability.go |
Analyze startup rejection and inventory evidence. |
pkg/cli/audit_model_availability_test.go |
Cover evidence handling, rendering, and caching. |
pkg/cli/audit_finding_codes.go |
Define model codes and playbook routes. |
pkg/cli/audit_cache.go |
Bump the audit cache version. |
pkg/cli/audit_cache_test.go |
Reject older cached audits. |
docs/src/content/docs/reference/audit.md |
Document findings and diagnostic links. |
actions/setup/js/safe_output_summary.test.cjs |
Cover diagnostic links and preserved outcomes. |
actions/setup/js/safe_output_summary.cjs |
Link diagnostic summaries to guidance. |
actions/setup/js/detect_agent_errors.test.cjs |
Cover runtime playbook-link conditions. |
actions/setup/js/detect_agent_errors.cjs |
Print runtime troubleshooting links. |
.github/skills/agentic-workflows/tests/test_guidance.py |
Enforce guidance coverage and routing contracts. |
.github/skills/agentic-workflows/tests/fixtures.json |
Add eight diagnostic-routing cases. |
.github/skills/agentic-workflows/SKILL.md |
List the new playbook. |
.github/aw/debug-playbook.md |
Consolidate code-specific diagnostic guidance. |
.github/aw/debug-model-engine-misconfiguration.md |
Link same-run availability checks. |
.github/aw/debug-agentic-workflow.md |
Update evidence collection and diagnostic routing. |
.changeset/minor-audit-model-availability-playbook.md |
Record the minor release change. |
Review effort: Max
| "strings" | ||
| ) | ||
|
|
||
| var piModelUnavailablePattern = regexp.MustCompile(`(?m)^\[gh-aw/pi-models-json\] fatal: Pi model "([a-zA-Z0-9][a-zA-Z0-9._/?=+-]{0,199})" is not advertised by the configured ([a-zA-Z0-9_-]{1,64}) proxy endpoint;`) |
| } | ||
| return nil | ||
| } | ||
| match := piModelUnavailablePattern.FindSubmatch(logData) |
| requested := modelNameWithoutProvider(strings.ToLower(model)) | ||
| requested, _, _ = strings.Cut(requested, "?") |
| for i := range findings { | ||
| findings[i].PlaybookURL = auditFindingPlaybookURL(findings[i].Code) | ||
| } |
|
🎉 This pull request is included in a new release. Release: |
Summary
gh aw auditto recognize Pi's fatal model-not-advertised startup error and analyze the exact model against the same run's completed, matching provider inventory. Emit stablemodel_unavailableormodel_availability_unknownfindings, bounded same-family inspection candidates with endpoint metadata, and actionable recommendations without automatic substitution or global-retirement claims.playbook_urlto audit findings and print relevant playbook links in console output, Actions agent-error logs, and safe-output diagnostic logs/step summaries. Preserve successful and intentionally skipped outcomes..github/aw/debug-playbook.md#model-availability, remove the standalone guide, and update all entry links. Cover gh-aw-owned audit, runtime, safe-output, merge, parser/ledger, and queue diagnostic codes with evidence/remedy mappings; handle open-ended external codes explicitly.Validation
make fmtandmake agent-report-progresspassed; final consolidated snapshot also passedmake agent-report-progress-no-test.claude-opus-5, lists same-runclaude-opus-5.5as an inspection candidate, and links the consolidated model section. Verified cachedgh aw modelsoutput separately.The complete guidance suite retains two pre-existing link-format assertion failures in
test_routing_and_evidence_links(local/embedded skill routers use absolute GitHub URLs while assertions require relative links). New guidance checks pass; unrelated routing formats were not changed.Related investigation
The rootless
/run/awf-config-auditpublication warning is separate from the fatal model rejection. Upstream issue filed as requested: github/gh-aw-firewall#10131. No upstream implementation changes are included.