refactor: extract shared pi-agent-core driver helpers to eliminate duplication#49665
Conversation
…r_helpers.cjs Move the duplicate `emitJsonl` and built-in `getApiKey` implementations into a new shared helper module `actions/setup/js/pi_agent_core_driver_helpers.cjs`. - pi_agent_core_driver.cjs now imports `emitJsonl` and `getBuiltinApiKey` from the helper; `buildGetApiKey` delegates to `getBuiltinApiKey` for the no-gateway path instead of repeating the switch statement. - The sample driver imports `emitJsonl` and `getApiKey` from the helper via a relative path, removing ~30 lines of duplicated code. Closes #49663 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. PR #49665 contains only production files (.cjs driver files). Test Quality Sentinel skipped. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #49665 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Extracts shared Pi driver utilities to reduce duplication between production and sample drivers.
Changes:
- Adds shared JSONL and API-key helpers.
- Updates both drivers to consume the shared module.
- Documents helper requirements for copied samples.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/pi_agent_core_driver.cjs |
Uses shared driver helpers. |
actions/setup/js/pi_agent_core_driver_helpers.cjs |
Defines shared JSONL and key-resolution utilities. |
.github/drivers/pi_agent_core_driver_sample_node.cjs |
Replaces duplicated helpers with an import. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — requesting changes on two small but meaningful issues before merge.
📋 Key Themes & Highlights
Key Themes
- Naming inconsistency:
getApiKeyis exported from the helpers module but aliased asgetBuiltinApiKeyat the main driver's import site. The helpers module should own the descriptive name. - Fragile require path in the sample driver: The
'../../actions/setup/js/...path is the most likely copy-paste failure for users, and the comment warns them — but apath.resolvecall gives a better error message and no extra cost.
Positive Highlights
- ✅ Clean extraction — the helpers module is well-documented and the split is correct
- ✅ No behaviour change; the
codexcase that was only inpi_agent_core_driver.cjsis now correctly included in the shared helper - ✅ Comment in sample driver actively guides copy-paste users
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 29.9 AIC · ⌖ 11.9 AIC · ⊞ 7.1K
Comment /matt to run again
| return undefined; | ||
| } | ||
| } | ||
| const { emitJsonl, getApiKey } = require("../../actions/setup/js/pi_agent_core_driver_helpers.cjs"); |
There was a problem hiding this comment.
[/codebase-design] The require('../../actions/setup/js/pi_agent_core_driver_helpers.cjs') path hard-codes a relative depth that breaks if either file moves. The header comment already warns users who copy the file — but this is a sample driver designed to be copied, so a stale relative path is the highest-probability failure mode. Consider replacing with path.resolve(__dirname, ...) for a clearer runtime error when the path is wrong.
💡 Example
const path = require('path');
const { emitJsonl, getApiKey } = require(
path.resolve(__dirname, '../../actions/setup/js/pi_agent_core_driver_helpers.cjs')
);This keeps the same semantics but produces a clear MODULE_NOT_FOUND with the resolved absolute path rather than a puzzling relative-path error.
@copilot please address this.
| * @returns {string|undefined} | ||
| */ | ||
| function getApiKey(provider) { | ||
| switch (provider) { |
There was a problem hiding this comment.
[/codebase-design] The exported name getApiKey does not signal that it only covers built-in providers. The main driver already aliases it as getBuiltinApiKey at the import site, which is the clearer name — rename the export to match so the public surface is self-documenting and the alias is unnecessary.
💡 Suggested rename
// pi_agent_core_driver_helpers.cjs
function getBuiltinApiKey(provider) { ... }
module.exports = { emitJsonl, getBuiltinApiKey };
// pi_agent_core_driver.cjs
const { emitJsonl, getBuiltinApiKey } = require('./pi_agent_core_driver_helpers.cjs');
// sample driver
const { emitJsonl, getBuiltinApiKey: getApiKey } = require('...');Consistent naming prevents future callers from confusing the helper with a fully generic key resolver.
@copilot please address this.
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 21 AIC · ⌖ 7.41 AIC · ⊞ 5.4K
| return undefined; | ||
| } | ||
| } | ||
| const { emitJsonl, getApiKey } = require("../../actions/setup/js/pi_agent_core_driver_helpers.cjs"); |
There was a problem hiding this comment.
The old inline getApiKey did not handle the "codex" provider string (it fell through to default: return undefined). The new shared helper adds case "codex": alongside case "openai":, so the sample driver now resolves CODEX_API_KEY/OPENAI_API_KEY for "codex" requests where it previously returned undefined. This is almost certainly the right alignment — confirm any callers that relied on the old undefined return for "codex" still behave correctly.
@copilot please address this.
There was a problem hiding this comment.
Verdict: Approve — clean deduplication refactor, no functional changes.
Extracted emitJsonl/getApiKey into a shared helper module consumed by both the built-in and sample drivers. Verified require("../../actions/setup/js/pi_agent_core_driver_helpers.cjs") resolves correctly from .github/drivers/, module exports load fine, and no .lock.yml files embed the old duplicated switch statements that would need updating.
💡 Review notes
- Behavior-preserving: switch statement logic for
emitJsonl/getApiKeyis copied verbatim into the new shared module. - Relative require path from the sample driver correctly walks up to
actions/setup/js/. - No test coverage changes needed since this is a pure extraction with identical logic.
- Comment update in the sample driver appropriately warns downstream copiers about the new dependency.
🔎 Code quality review by PR Code Quality Reviewer · auto · 80.6 AIC · ⌖ 5.17 AIC · ⊞ 7.8K
Comment /review to run again
|
@copilot quick triage for this PR:
Run details: https://github.com/github/gh-aw/actions/runs/30722880612
|
|
@copilot quick triage for this PR:
Run details: https://github.com/github/gh-aw/actions/runs/30726226343
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the open review feedback in acff48f: the sample driver now resolves the helper via |
🔍 Triage Summary
Extracts shared pi-agent-core driver helpers. CHANGES_REQUESTED from automated reviewer — needs rework. Automated triage — see the triage report for full context.
|
pi_agent_core_driver_sample_node.cjsduplicatedemitJsonl,getApiKey, and related logic verbatim frompi_agent_core_driver.cjs, creating a drift risk where any change to JSONL formatting or provider key resolution had to be applied in two places.Changes
New
actions/setup/js/pi_agent_core_driver_helpers.cjs— shared module exportingemitJsonlandgetApiKey(built-in provider resolution); single source of truth for both drivers.actions/setup/js/pi_agent_core_driver.cjs— importsemitJsonlandgetBuiltinApiKeyfrom helpers;buildGetApiKeydelegates togetBuiltinApiKeyfor the no-gateway path instead of repeating the switch..github/drivers/pi_agent_core_driver_sample_node.cjs— replaces the two duplicated functions with a singlerequire(). Comment updated to guide users who copy the file to repos without gh-aw action sources.Run details: https://github.com/github/gh-aw/actions/runs/30726226343