Add Pydantic AI shared engine definition and smoke test - #51161
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot add pydantic-ai to engines.json |
There was a problem hiding this comment.
Pull request overview
Adds a first-class Pydantic AI engine definition and smoke workflow.
Changes:
- Defines installation, execution, MCP, networking, and log parsing.
- Adds a smoke workflow and slash-command routing.
- Generates the corresponding workflow lock file.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/shared/pydantic.md |
Defines the Pydantic AI engine. |
.github/workflows/smoke-pydantic.md |
Adds engine smoke testing. |
.github/workflows/smoke-pydantic.lock.yml |
Compiles the smoke workflow. |
.github/workflows/agentic_commands.yml |
Registers /smoke-pydantic. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/4 changed files
- Comments generated: 1
- Review effort level: Balanced
| 1. **File Writing Testing**: Create a test file `/tmp/gh-aw/agent/smoke-test-pydantic-${{ github.run_id }}.txt` with content "Smoke test passed for Pydantic AI" (create the directory if it doesn't exist) | ||
| 2. **Bash Tool Testing**: Execute bash commands to verify file creation was successful (use `cat` to read the file back) | ||
| 3. **Repository Access Testing**: Run `git log --oneline -1` in the repository checkout and confirm a commit is reported |
| - .pydantic-ai/ | ||
| network: | ||
| defaults: | ||
| - host.docker.internal |
There was a problem hiding this comment.
@copilot none of these should be needed since pydantic is already pre-installed. keep network as "defaults"
There was a problem hiding this comment.
Updated in d36df30. I removed the Pydantic install pre-step and switched the shared engine networking back to defaults (dropping the extra PyPI domains).
|
No test files were added or modified in this PR. Test Quality Sentinel skipped. PR contains only workflow definitions (.md, .yml, .lock.yml) and no behavioral tests (Go *_test.go or JavaScript .test.cjs/.test.js). |
|
No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions detected in default business logic paths). |
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — commenting (no blocking issues, but a few items worth addressing before promoting from experimental).
📋 Key Themes & Highlights
Key Themes
- Runtime network policy:
smoke-pydantic.mdsetsnetwork: allowed: []which may conflict with the engine's need to reach the LLM proxy. Needs verification againstsmoke-aider.mdbaseline. - Log-parser token extraction: Silent zero-token totals when Pydantic AI emits usage under a nested key (e.g.
usage.input_tokens). A small fixture-based test would prevent regressions. - Empty
tool_resultcontent:msgcan be empty string; downstream schema validators may reject it. - Network hosts at runtime vs install-time: PyPI entries in the runtime allowlist are only needed during
pre-agent-steps. - Version override discoverability: Override pattern documented only in an HTML comment; a YAML-adjacent comment would be more visible.
Positive Highlights
- ✅ Clean
log-parserstructure mirroring the aider pattern - ✅
secret-strategy: universal-llm-consumercorrectly routes through AWF proxy - ✅ MCP config via
GH_AW_MCP_CONFIGis consistent with the existing engine contract - ✅ Smoke test structure mirrors
smoke-aider.md— easy to compare and audit
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 38.3 AIC · ⊞ 7.1K
Comment /matt to run again
| - .pydantic-ai/ | ||
| network: | ||
| defaults: | ||
| - host.docker.internal |
There was a problem hiding this comment.
[/codebase-design] Network defaults includes pypi.org and files.pythonhosted.org — these are only needed during pre-agent-steps installation, not agent runtime.
💡 Suggestion
PyPI hosts are consumed by pip install in pre-agent-steps, which runs before the agent sandbox. If the runtime network policy cannot be scoped per-phase, document why these hosts must remain open during agent execution. Also, the PR comment from @pelikhan suggests pydantic-ai-harness may already be pre-installed, which would make the pip install step and these entries unnecessary entirely.
@copilot please address this.
| if (parsed.input_tokens) inputTokens += parsed.input_tokens; | ||
| if (parsed.output_tokens) outputTokens += parsed.output_tokens; | ||
| const entryType = parsed.type != null ? String(parsed.type) : "log"; | ||
| const msg = parsed.msg || parsed.message || parsed.content || ""; |
There was a problem hiding this comment.
[/tdd] The log-parser only accumulates input_tokens / output_tokens from JSON lines that have those exact keys — if Pydantic AI emits them under a different key (e.g. usage.input_tokens) the totals silently remain zero.
💡 Suggestion
Add a test fixture (a small JSONL sample) and a unit test that asserts:
- known token-bearing lines are counted correctly
- lines with a nested
usageobject are also handled - the final
resultentry reflects the correct totals
Without this, a Pydantic AI output format change will produce invisible zero-token audit entries.
@copilot please address this.
| const toolName = parsed.tool || parsed.name || entryType; | ||
| logEntries.push({ type: "assistant", message: { content: [{ type: "tool_use", id: toolId, name: toolName, input: {} }] } }); | ||
| logEntries.push({ type: "user", message: { content: [{ type: "tool_result", tool_use_id: toolId, content: msg }] } }); | ||
| } else if (msg) { |
There was a problem hiding this comment.
[/codebase-design] Tool-result entries always set content: msg where msg may be an empty string — the harness may reject a tool_result with an empty content array.
💡 Suggestion
Use a fallback: content: msg || '(no output)' to guarantee a non-empty string. Downstream log consumers that validate against the Anthropic message schema will reject empty content.
@copilot please address this.
| id: pydantic-ai | ||
| strict: true | ||
| imports: | ||
| - shared/pydantic.md |
There was a problem hiding this comment.
[/codebase-design] network: allowed: [] blocks all outbound network access, but the smoke test calls git log which may need to resolve remote refs depending on the checkout strategy. More critically, pai needs to reach the LLM proxy at api.githubcopilot.com.
💡 Suggestion
Compare with smoke-aider.md to confirm whether the smoke test inherits the shared engine network policy or fully overrides it. If allowed: [] is an override, the agent will be unable to reach any LLM endpoint and the smoke test will fail at the engine invocation step.
@copilot please address this.
| version: "3.12" | ||
| pre-agent-steps: | ||
| - name: Install Pydantic AI CLI | ||
| run: | |
There was a problem hiding this comment.
[/codebase-design] engine.version: "0.1.0" is hard-coded in the shared definition. When the shared file is imported, users who want to override the version must redeclare engine.version in their own workflow — but the precedence of that override is not documented.
💡 Suggestion
The PR description explains version override syntax, but that comment lives only in the HTML comment block at the bottom of the file. Move a one-line note into the visible YAML, or add a comment adjacent to the version key:
engine:
id: pydantic-ai
version: "0.1.0" # override in your workflow: engine: { version: "x.y.z" }This keeps the contract discoverable without reading the HTML comment.
@copilot please address this.
There was a problem hiding this comment.
Review: Add Pydantic AI shared engine definition and smoke test
The shared engine definition is well-structured and consistent with existing engines (aider, goose). Two existing review threads cover MCP smoke-test coverage and unnecessary pip installs. One new issue found:
Log-parser: MCP failure lines leak into the assistant transcript — lines matching the MCP failure regex are added to mcpFailures but not continued, so they also fall through to pendingText.push(line.trim()) and appear as synthetic assistant text. See inline comment on line 103 of shared/pydantic.md.
Overall the PR is directionally sound. Blocking the MCP failure line leak before merge is recommended.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 62.2 AIC · ⊞ 5.5K
| pendingText.push(msg); | ||
| } | ||
| } else { | ||
| pendingText.push(line.trim()); |
There was a problem hiding this comment.
MCP failure lines match the mcpFailures push above but are not skipped afterward — they fall through to pendingText.push(line.trim()) and are emitted as synthetic assistant text in the log transcript. Every MCP error message ends up double-counted: once in mcpFailures and once as an assistant turn.
Fix by adding continue after the mcpFailures.push(...) call:
if (/MCP server .* failed|MCP.*connection.*error|Failed to connect to MCP/i.test(line)) {
const serverMatch = line.match(/MCP server ['"x]?([^\s'"]+)['"x]?/i);
mcpFailures.push(serverMatch ? serverMatch[1] : line.trim());
continue; // prevent double-emit as transcript text
}@copilot please address this.
|
fyi, this will be valuable to have in place already, but we have some more work to do on the edit: some more info
|
|
@copilot please refresh the branch if needed and run the
|
|
@dsfaccini we don't plan to own this long term but it is easier to iterate here to flush out bugs in gh-aw, then glabdly transfer and keep a copy. |
|
(claude code compatibility is smart because a lot of tooling will "just work") |
|
This PR will be merged probably a bit broken because I need a "smoke-pydantic" action in main in order to run it. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in 9ca8622. The shared Pydantic engine now runs through
Done in 9ca8622. I refreshed the branch context, addressed the remaining actionable item ( |
|
@copilot add a step using uv api version to predownload pai |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Pydantic AI wants to dogfood their own stack as a gh-aw engine. This adds the shared definition and smoke test to support
engine: id: pydantic-aias a first-class importable engine.shared/pydantic.mdShared engine definition following the
aider.mdpre-agent-steps pattern (used becausebehaviors.installation.package-manager: uvsilently emits no install step):pydantic-ai-harnessvia pip inpre-agent-steps; version pinned viaengine.versionsecret-strategy: universal-llm-consumer— routes through AWF proxyGH_AW_MCP_CONFIGso safe outputs flow throughsafeoutputsautomaticallylog-parserfunction that normalizes Pydantic AI JSONL output into conversation events, enabling step summaries and token tracking ingh aw logs/gh aw auditsmoke-pydantic.mdSmoke test workflow mirroring
smoke-aider.md: validates file writing, bash execution, and repo access, then emits results as an issue + PR comment via$GH_AW_SAFE_OUTPUTS.