Conversation
…mport-schema Agent-Logs-Url: https://github.com/github/gh-aw/sessions/58293861-6689-4b35-a114-76ee2ff1ceb3 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…in parseQmdTool Agent-Logs-Url: https://github.com/github/gh-aw/sessions/58293861-6689-4b35-a114-76ee2ff1ceb3 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Reimplements tools.qmd as a single parameterized shared import (shared/qmd.md) using import-schema, replacing the previous hardcoded shared presets and updating workflows accordingly.
Changes:
- Added
shared/qmd.mdwithimport-schema-driven parameters forcheckouts,searches,cache-key,gpu, andruns-on. - Updated multiple workflows to import
shared/qmd.md(and regenerated corresponding.lock.ymlmanifests). - Extended
main_workflow_schema.jsonto allowwith:parameters to include arrays containing object elements; updatedparseQmdToolto ignore unexpanded import-schema placeholders.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/tools_parser.go | Adds guards for qmd cache-key / runs-on to avoid treating unsubstituted import placeholders as literals. |
| pkg/parser/schemas/main_workflow_schema.json | Allows imports[].with to pass arrays that include objects (needed for qmd checkouts / searches). |
| .github/workflows/shared/qmd.md | New parameterized shared qmd import with import-schema and qmd guidance text. |
| .github/workflows/shared/mcp/qmd.md | Removes legacy hardcoded qmd preset. |
| .github/workflows/shared/mcp/qmd-docs.md | Removes legacy hardcoded qmd-docs preset. |
| .github/workflows/weekly-blog-post-writer.md | Switches qmd setup to shared/qmd.md with explicit parameters. |
| .github/workflows/weekly-blog-post-writer.lock.yml | Regenerated lock manifest reflecting new qmd import. |
| .github/workflows/unbloat-docs.md | Switches qmd setup to shared/qmd.md with explicit parameters. |
| .github/workflows/unbloat-docs.lock.yml | Regenerated lock manifest reflecting new qmd import. |
| .github/workflows/technical-doc-writer.md | Switches qmd setup to shared/qmd.md with explicit parameters. |
| .github/workflows/technical-doc-writer.lock.yml | Regenerated lock manifest reflecting new qmd import. |
| .github/workflows/smoke-codex.md | Replaces inline tools.qmd config with shared/qmd.md import parameters. |
| .github/workflows/smoke-codex.lock.yml | Regenerated lock manifest reflecting new qmd import. |
| .github/workflows/glossary-maintainer.md | Switches qmd setup to shared/qmd.md with explicit parameters. |
| .github/workflows/glossary-maintainer.lock.yml | Regenerated lock manifest reflecting new qmd import. |
| .github/workflows/dictation-prompt.md | Switches qmd setup to shared/qmd.md with explicit parameters. |
| .github/workflows/dictation-prompt.lock.yml | Regenerated lock manifest reflecting new qmd import. |
| .github/workflows/developer-docs-consolidator.md | Switches qmd setup to shared/qmd.md with explicit parameters. |
| .github/workflows/developer-docs-consolidator.lock.yml | Regenerated lock manifest reflecting new qmd import. |
| .github/workflows/dev.md | Switches from inline tools.qmd to shared/qmd.md import parameters. |
| .github/workflows/dev.lock.yml | Regenerated lock manifest reflecting new qmd import. |
| .github/workflows/daily-doc-updater.md | Switches qmd setup to shared/qmd.md with explicit parameters. |
| .github/workflows/daily-doc-updater.lock.yml | Regenerated lock manifest reflecting new qmd import. |
| .github/workflows/daily-doc-healer.md | Switches qmd setup to shared/qmd.md with explicit parameters. |
| .github/workflows/daily-doc-healer.lock.yml | Regenerated lock manifest reflecting new qmd import. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/workflow/tools_parser.go
Outdated
| // Handle cache-key field. Skip values that contain unexpanded import-schema expressions | ||
| // (e.g. "${{ github.aw.import-inputs.cache-key }}") which are left as literal strings | ||
| // when the caller does not supply the optional input. | ||
| if cacheKey, ok := configMap["cache-key"].(string); ok && cacheKey != "" && !strings.Contains(cacheKey, "${{") { | ||
| config.CacheKey = cacheKey | ||
| toolsParserLog.Printf("qmd tool cache-key: %s", cacheKey) |
There was a problem hiding this comment.
The new guard skips any cache-key containing "${{", but cache keys commonly (and per qmd.go docs) legitimately include GitHub Actions expressions like qmd-index-${{ hashFiles('docs/**') }}. This will silently drop user-provided cache keys and change behavior. Consider only filtering the specific unexpanded import-schema placeholder (e.g. strings that reference github.aw.import-inputs.cache-key, or exact-match the placeholder), while allowing other ${{ ... }} expressions through unchanged.
| // Handle runs-on field (override runner image for the indexing job). Skip values that | ||
| // contain unexpanded import-schema expressions which are left as literal strings when | ||
| // the caller does not supply the optional input. | ||
| if runsOnVal, exists := configMap["runs-on"]; exists { | ||
| if runsOnStr, ok := runsOnVal.(string); ok && runsOnStr != "" { | ||
| if runsOnStr, ok := runsOnVal.(string); ok && runsOnStr != "" && !strings.Contains(runsOnStr, "${{") { | ||
| config.RunsOn = runsOnStr | ||
| toolsParserLog.Printf("qmd tool runs-on: %s", runsOnStr) |
There was a problem hiding this comment.
Same issue as cache-key: skipping any runs-on value containing "${{" will also drop legitimate GitHub Actions expressions (e.g. matrix/inputs-based runner selection). If the intent is to ignore only unexpanded import-schema placeholders, narrow the check to github.aw.import-inputs.runs-on (or exact placeholder matching) rather than all expressions.
|
@copilot review comments |
…rt-inputs. prefix Agent-Logs-Url: https://github.com/github/gh-aw/sessions/501188f0-c734-4ea3-8f94-7657e08f0135 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in 859cb0a. Replaced the over-broad |
|
❌ Tool validation failed! Agent Container Smoke Test detected missing tools: failed |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
|
💫 TO BE CONTINUED... Smoke Claude failed! Our hero faces unexpected challenges... |
|
📰 DEVELOPING STORY: Smoke Copilot reports failed. Our correspondents are investigating the incident... |
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
💥 [THE END] — Illustrated by Smoke Claude
| @@ -50,7 +50,17 @@ timeout-minutes: 45 | |||
|
|
|||
There was a problem hiding this comment.
The new shared/qmd.md import with runs-on: aw-gpu-runner-T4 looks good. Consider documenting why GPU is required for this doc healer workflow to help future maintainers understand the requirement.
| - uses: shared/qmd.md | ||
| with: | ||
| runs-on: aw-gpu-runner-T4 | ||
| gpu: true |
There was a problem hiding this comment.
The ignore patterns for .git/** and node_modules/** are sensible defaults. If these become commonly needed across workflows, consider centralizing them as defaults in the shared qmd.md import schema to reduce repetition.
There was a problem hiding this comment.
Agreed — centralizing ignore defaults in shared/qmd.md would reduce boilerplate across workflows. Worth a follow-up once the import schema pattern stabilizes.
📰 BREAKING: Report filed by Smoke Copilot
|
@copilot merge main and recompile |
…ools-shared-agentic-workflow # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 5080790 — merged Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/f5d723aa-88c2-477f-9e36-b7f57d11aecb Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in abada22 — updated the three WASM golden files ( |
|
✅ All tools validated successfully! Agent Container Smoke Test confirms agent container is ready. |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS
|
|
Smoke Test: Copilot - 23699095779
Overall: PR Author:
|
There was a problem hiding this comment.
Quick smoke-test review pass. The isUnexpandedImportInput helper is a clean approach to guard against unexpanded import-schema placeholders. The existing review comments on the broader ${{ checks are valid — using the specific placeholder guard is the right fix.
📰 BREAKING: Report filed by Smoke Copilot
| // (exactly "${{ github.aw.import-inputs.cache-key }}") left as literal strings when | ||
| // the caller does not supply the optional input. Legitimate GitHub Actions expressions | ||
| // such as "qmd-${{ hashFiles('docs/**') }}" are kept as-is. | ||
| if cacheKey, ok := configMap["cache-key"].(string); ok && cacheKey != "" && !isUnexpandedImportInput(cacheKey) { |
There was a problem hiding this comment.
Using !isUnexpandedImportInput(cacheKey) here is the correct fix, but the previous broader check (strings.Contains(s, "$\{\{")) would have incorrectly dropped valid expressions like qmd-$\{\{ hashFiles('docs/**') }}. Good that this narrower guard is already in place.
| // because the caller did not supply the optional input. It returns false for any other | ||
| // value, including legitimate GitHub Actions expressions like "${{ hashFiles('...') }}". | ||
| func isUnexpandedImportInput(s string) bool { | ||
| return strings.Contains(s, "github.aw.import-inputs.") |
There was a problem hiding this comment.
The isUnexpandedImportInput helper is a clean, focused guard for detecting unexpanded github.aw.import-inputs.* placeholders. This is the right approach — narrow check, avoids the over-broad strings.Contains(s, "$\{\{") pattern flagged in the sibling cache-key/runs-on checks.
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
Smoke Test Results — Run §23699095768Overall: PARTIAL (core tests ✅, some PR tests
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
💥 [THE END] — Illustrated by Smoke Claude
| @@ -50,7 +50,17 @@ timeout-minutes: 45 | |||
|
|
|||
| imports: | |||
There was a problem hiding this comment.
The uses: shared/qmd.md import syntax with with: block is a nice improvement over the flat - shared/mcp/qmd-docs.md form. The explicit GPU runner and checkout configuration makes the intent clearer.
| - shared/mcp/qmd-docs.md | ||
| - uses: shared/qmd.md | ||
| with: | ||
| runs-on: aw-gpu-runner-T4 |
There was a problem hiding this comment.
Good use of checkouts config with a targeted glob pattern **/*.{md,mdx}. The context field describing the purpose of the checkout will help the QMD search tool understand what's indexed.
tools.qmdwas only accessible via two hardcoded non-parameterized shared presets (shared/mcp/qmd.md,shared/mcp/qmd-docs.md). This replaces them with a single parameterizedshared/qmd.mdusingimport-schema, making qmd configuration composable and explicitly configurable per workflow.New
shared/qmd.mdExposes all qmd fields via typed
import-schemaparameters:All parameters are optional — unset inputs leave expression placeholders that
parseQmdToolnow silently ignores.Changes
shared/mcp/qmd.mdandshared/mcp/qmd-docs.mdshared/qmd.mdwithimport-schemaforcheckouts,searches,cache-key,gpu,runs-ondev.md,smoke-codex.md, and 8 doc workflows to use the new importmain_workflow_schema.json): extendedwith:to allow arrays of objects, not just primitives — required for passingcheckouts/searchesaswithparametersparseQmdTool: guardscache-keyandruns-onagainst unsubstituted${{...}}expression strings usingstrings.Containsto avoid them being treated as literal values when optional inputs are omitted✨ PR Review Safe Output Test - Run 23698148563
✨ PR Review Safe Output Test - Run 23699095768