Skip to content

feat: add power-platform-connectors type to /new-repo - #1

Merged
PBNZ merged 10 commits into
mainfrom
feat/power-platform-connectors
Jul 3, 2026
Merged

feat: add power-platform-connectors type to /new-repo#1
PBNZ merged 10 commits into
mainfrom
feat/power-platform-connectors

Conversation

@PBNZ

@PBNZ PBNZ commented Jul 3, 2026

Copy link
Copy Markdown
Owner

Adds the power-platform-connectors repo type to /new-repo: a committed Postman collection → OpenAPI 2.0 custom-connector definitions for Microsoft Power Platform (2.0 + < 1 MB are hard requirements, verified from MS Learn).

What it ships (type overlay, Core + Public)

  • A pinned Docker toolchain (Node 18 + postman-to-openapi + api-spec-converter + swagger-cli) and generate.mjs: Postman → OpenAPI 3.0 → Swagger 2.0, normalised to valid 2.0, split per top-level folder only when a single definition would hit the 1 MB limit, self-validating (fails on invalid or oversize).
  • CI (regenerate + validate on source change) and a scheduled sync workflow (hash-diff upstream → opens a PR with regenerated defs, validated inline).

Why the normalisation

The 3.0→2.0 downconvert is lossy. Proven against a real public collection (USPS), which failed validation four ways before fixups: unresolved {{vars}} (no host), mis-mapped apikey auth, missing response descriptions, undeclared path params. All fixed; USPS now yields a valid < 1 MB definition.

Verified

5 repo validators green; scaffold resolution + precedence correct; the stamped repo's Docker pipeline produces valid Swagger 2.0 from the real USPS collection.

Notes for users of the type

  • Sync PRs need Settings → Actions → Allow GitHub Actions to create and approve pull requests.
  • The conversion is lossy — review before importing to Power Platform.

Registered in the /new-repo SKILL enum, the standard's Types table, the testing matrix (+ backfilled the missing docker-compose row), the README, and docs/adr/0003.

A new repo type that turns a committed Postman collection into OpenAPI 2.0
custom-connector definitions for Microsoft Power Platform (2.0 + <1MB are hard
requirements — verified from MS Learn).

- Pinned-Docker generator (Node 18 + postman-to-openapi + api-spec-converter +
  swagger-cli): converts Postman -> OpenAPI 3.0 -> Swagger 2.0, normalises the
  lossy output to valid 2.0 (resolves {{vars}} for host/basePath, derives
  securityDefinitions from the Postman auth, backfills response descriptions,
  adds missing path params), splits per top-level folder ONLY when a single def
  would hit 1MB, and self-validates every output (fails on invalid or oversize).
- CI regenerates + validates on source change; a scheduled sync workflow detects
  upstream changes (hash-diff), regenerates, validates inline (the bot PR won't
  trigger CI), and opens a PR. Committed source -> builds with just Docker, no
  Postman account; account-free sync when the source has a public URL.
- Core + Public tier (no Published — connectors are imported manually). Proven
  end-to-end against a real public Postman collection. Registered in the SKILL
  enum, the standard's Types table, the testing matrix (+ backfilled the missing
  docker-compose row), the README, and docs/adr/0003.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DXtxaAQHcsDThJ6BaqwXES

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new power-platform-connectors repository type that automates the conversion of a committed Postman collection into valid OpenAPI 2.0 (Swagger) custom-connector definitions for Microsoft Power Platform. The implementation includes a pinned Docker toolchain, a generator script that normalizes and splits definitions exceeding 1 MB, and workflows for CI and daily synchronization. The review feedback highlights several critical improvements to make the pipeline more robust, such as ensuring the source directory exists before file operations, replacing unsafe string-based variable resolution with recursive object traversal, capturing and reporting validation errors, and using a static branch name in the sync workflow to avoid accumulating stale pull requests.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

const warnings = [];

// --- locate + load the committed collection ---
const srcName = readdirSync('source').find((f) => f.endsWith('.json'));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If the source directory does not exist (e.g., when the repository is freshly scaffolded), readdirSync('source') will throw an ENOENT error and crash the generator. Check if the directory exists and create it if necessary.

if (!existsSync('source')) {
  mkdirSync('source', { recursive: true });
}
const srcName = readdirSync('source').find((f) => f.endsWith('.json'));

Comment on lines +40 to +44
function resolveVars(obj, vars) {
let s = JSON.stringify(obj);
for (const v of vars) if (v && v.key && v.value) s = s.split(`{{${v.key}}}`).join(v.value);
return JSON.parse(s);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Performing string replacement directly on the serialized JSON string is unsafe and can easily produce invalid JSON if any variable value contains special characters (such as double quotes, backslashes, or newlines). Additionally, checking if (v && v.key && v.value) will incorrectly skip replacing variables with falsy but valid values like 0, false, or "". A recursive traversal of the parsed object is much safer and correctly handles all valid values.

function resolveVars(obj, vars) {
  if (typeof obj === 'string') {
    let s = obj;
    for (const v of vars) {
      if (v && v.key && v.value !== undefined && v.value !== null) {
        s = s.split(`{{${v.key}}}`).join(String(v.value));
      }
    }
    return s;
  }
  if (Array.isArray(obj)) {
    return obj.map((item) => resolveVars(item, vars));
  }
  if (obj && typeof obj === 'object') {
    const res = {};
    for (const [k, v] of Object.entries(obj)) {
      res[k] = resolveVars(v, vars);
    }
    return res;
  }
  return obj;
}

Comment on lines +54 to +55
SRC=$(ls source/*.json 2>/dev/null | head -1); SRC=${SRC:-source/collection.json}
cp /tmp/coll.json "$SRC"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If the source directory does not exist in the repository, copying the fetched collection will fail. Ensure the directory is created before copying.

          mkdir -p source
          SRC=$(ls source/*.json 2>/dev/null | head -1); SRC=${SRC:-source/collection.json}
          cp /tmp/coll.json "$SRC"

//
// Runs inside the pinned Docker image (p2o + api-spec-converter on PATH).

import { readFileSync, writeFileSync, mkdirSync, readdirSync, rmSync } from 'node:fs';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Import existsSync from node:fs to safely check if the source directory exists before reading it.

Suggested change
import { readFileSync, writeFileSync, mkdirSync, readdirSync, rmSync } from 'node:fs';
import { readFileSync, writeFileSync, mkdirSync, readdirSync, rmSync, existsSync } from 'node:fs';

Comment on lines +124 to +125
execFileSync('p2o', [pin, '-f', oas], { stdio: 'pipe' });
const raw = execFileSync('api-spec-converter', ['--from=openapi_3', '--to=swagger_2', '--syntax=json', oas], { stdio: 'pipe' });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Inherit stdio for the generator commands to allow warnings and errors from p2o and api-spec-converter to be printed directly to the console. This significantly improves debuggability when the conversion fails.

Suggested change
execFileSync('p2o', [pin, '-f', oas], { stdio: 'pipe' });
const raw = execFileSync('api-spec-converter', ['--from=openapi_3', '--to=swagger_2', '--syntax=json', oas], { stdio: 'pipe' });
execFileSync('p2o', [pin, '-f', oas], { stdio: 'inherit' });
const raw = execFileSync('api-spec-converter', ['--from=openapi_3', '--to=swagger_2', '--syntax=json', oas], { stdio: ['pipe', 'pipe', 'inherit'] });

Comment on lines +150 to +152
let valid = true;
try { execFileSync('swagger-cli', ['validate', file], { stdio: 'pipe' }); } catch { valid = false; }
written.push({ file, bytes, over: bytes >= LIMIT, valid });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When swagger-cli validate fails, the error output is currently swallowed, leaving the user with no details on why the Swagger definition is invalid. Capture and report the validation error to the warnings list.

  let valid = true;
  try {
    execFileSync('swagger-cli', ['validate', file], { stdio: 'pipe' });
  } catch (err) {
    valid = false;
    warnings.push(`Validation failed for ${file}:\n${err.stderr?.toString() || err.message}`);
  }

Comment on lines +74 to +83
BR="sync/postman-$(date -u +%Y%m%d%H%M%S)"
git config user.name "github-actions[bot]"
git config user.email "41898282+github-actions[bot]@users.noreply.github.com"
git checkout -b "$BR"
git add -A
git commit -m "chore: sync connector definitions from upstream Postman collection"
git push -u origin "$BR"
gh pr create --base main --head "$BR" \
--title "Sync connector definitions from upstream Postman collection" \
--body "The upstream Postman collection changed. Regenerated OpenAPI 2.0 definitions (validated as Swagger 2.0, each < 1 MB). **Review before importing to Power Platform** — the Postman-to-2.0 conversion is lossy."

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using a timestamped branch name creates a new branch and a new PR on every daily run if the upstream collection changes frequently. This leads to an accumulation of stale branches and PRs. Using a static branch name like sync/postman and force-pushing updates the existing PR cleanly. Additionally, appending || true to gh pr create prevents the workflow from failing if the PR is already open.

          BR="sync/postman"
          git config user.name  "github-actions[bot]"
          git config user.email "41898282+github-actions[bot]@users.noreply.github.com"
          git checkout -B "$BR"
          git add -A
          git commit -m "chore: sync connector definitions from upstream Postman collection"
          git push -f -u origin "$BR"
          gh pr create --base main --head "$BR" \
            --title "Sync connector definitions from upstream Postman collection" \
            --body "The upstream Postman collection changed. Regenerated OpenAPI 2.0 definitions (validated as Swagger 2.0, each < 1 MB). **Review before importing to Power Platform** — the Postman-to-2.0 conversion is lossy." || true

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new /new-repo repo type overlay, power-platform-connectors, intended for repositories that commit a Postman collection and generate Power Platform–importable OpenAPI 2.0 / Swagger 2.0 connector definitions via a pinned Docker toolchain, with CI + scheduled upstream sync.

Changes:

  • Document and register the new power-platform-connectors type across the standard docs, /new-repo skill inputs, ADRs, and changelog.
  • Add a Core template with a pinned Docker generator toolchain + scripts/generate.mjs that converts/normalizes Postman → OAS3 → Swagger 2.0 and enforces Swagger validity + < 1 MB per output.
  • Add Public workflows for CI regeneration/validation and a scheduled sync workflow that fetches upstream collections and opens regeneration PRs.

Reviewed changes

Copilot reviewed 13 out of 15 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
README.md Adds power-platform-connectors to the “built out” repo types list.
plugins/repokit/skills/repo-standard/standard/the-standard.md Adds a new type row describing the new repo type behavior and constraints.
plugins/repokit/skills/repo-standard/standard/testing-matrix.md Adds required checks for docker-compose and power-platform-connectors.
plugins/repokit/skills/new-repo/templates/types/power-platform-connectors/public/.github/workflows/sync.yml.tmpl Adds scheduled sync workflow that detects upstream changes and opens PRs with regenerated outputs.
plugins/repokit/skills/new-repo/templates/types/power-platform-connectors/public/.github/workflows/ci.yml.tmpl Adds CI workflow to build the pinned image and run the generator.
plugins/repokit/skills/new-repo/templates/types/power-platform-connectors/core/source/.gitkeep Seeds the committed source/ directory.
plugins/repokit/skills/new-repo/templates/types/power-platform-connectors/core/scripts/generate.mjs Implements the Postman → Swagger 2.0 generation/normalization/splitting/validation pipeline.
plugins/repokit/skills/new-repo/templates/types/power-platform-connectors/core/README.md.tmpl Documents usage, CI, sync workflow, and caveats for stamped repos.
plugins/repokit/skills/new-repo/templates/types/power-platform-connectors/core/Dockerfile Pins Node 18 and conversion tooling in Docker.
plugins/repokit/skills/new-repo/templates/types/power-platform-connectors/core/connectors/.gitkeep Seeds the output directory.
plugins/repokit/skills/new-repo/templates/types/power-platform-connectors/core/connectors.config.json Adds a configurable upstream sourceUrl, size limit, and output dir.
plugins/repokit/skills/new-repo/templates/types/power-platform-connectors/core/.postman/manifest.json Adds sync manifest placeholders for hash-based change detection.
plugins/repokit/skills/new-repo/SKILL.md Adds the new type to the /new-repo type input list and marks it as “built out”.
docs/adr/0003-power-platform-connectors-type.md Introduces ADR for the new type’s constraints and design decisions.
CHANGELOG.md Notes the addition of the new repo type and its key characteristics.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +11 to +20
import { readFileSync, writeFileSync, mkdirSync, readdirSync, rmSync } from 'node:fs';
import { execFileSync } from 'node:child_process';
import { join } from 'node:path';
import { tmpdir } from 'node:os';

const cfg = JSON.parse(readFileSync('connectors.config.json', 'utf8'));
const LIMIT = cfg.sizeLimitBytes ?? 1048576;
const OUT = cfg.output ?? 'connectors';
const work = tmpdir();
const warnings = [];
Comment on lines +40 to +44
function resolveVars(obj, vars) {
let s = JSON.stringify(obj);
for (const v of vars) if (v && v.key && v.value) s = s.split(`{{${v.key}}}`).join(v.value);
return JSON.parse(s);
}
Comment on lines +23 to +28
const srcName = readdirSync('source').find((f) => f.endsWith('.json'));
if (!srcName) {
console.log('source/ has no *.json collection yet — export your Postman collection there. Nothing to do.');
process.exit(0);
}
const rawObj = JSON.parse(readFileSync(join('source', srcName), 'utf8'));
Comment on lines +54 to +55
SRC=$(ls source/*.json 2>/dev/null | head -1); SRC=${SRC:-source/collection.json}
cp /tmp/coll.json "$SRC"
PBNZ and others added 2 commits July 4, 2026 00:43
…ygiene

From Copilot review of PR #1:
- generate.mjs: use a unique mkdtemp work dir (removed on exit) instead of a
  predictable path in the shared tmpdir.
- generate.mjs: resolve collection variables whose value is set including "" and
  "0" (check != null, not truthiness).
- generate.mjs + sync.yml: standardise on source/collection.json — the generator
  prefers it, and sync writes it — so the two never disagree when multiple JSONs
  exist. Re-proven: USPS collection -> valid Swagger 2.0; a 50 KB split limit ->
  25 valid < 1 MB definitions.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DXtxaAQHcsDThJ6BaqwXES
…safety

- generate.mjs: guard a missing source/ dir (existsSync) instead of crashing;
  rewrite resolveVars to recurse the parsed object and substitute inside string
  values (safe against quotes/backslashes/newlines in a variable value — a raw
  string-replace on serialized JSON could yield invalid JSON); surface
  p2o/api-spec-converter stderr and capture swagger-cli validation errors into the
  warnings for debuggability.
- sync.yml: mkdir -p source/.postman before writing; reuse one sync/postman branch
  with force-push (+ `|| true` on gh pr create) so daily runs update the existing
  PR instead of piling up new branches/PRs.

Re-proven: USPS -> valid Swagger 2.0; a special-char variable value resolves
safely; missing source/ exits cleanly; workflows parse as YAML.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DXtxaAQHcsDThJ6BaqwXES

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 15 changed files in this pull request and generated 2 comments.

}

// --- report ---
for (const w of written) console.log(`${(w.bytes / 1024).toFixed(1)} KB ${w.file}${w.over ? ' ** OVER 1MB **' : ''}${w.valid ? '' : ' ** INVALID Swagger 2.0 **'}`);
Comment on lines +192 to +196
if (bad.length) {
console.error(`\n${bad.length} definition(s) are over 1 MB or not valid Swagger 2.0 — split that folder further, trim the collection, or fix the source. See flags above.`);
process.exit(2);
}
console.log(`\n${written.length} connector definition(s) written to ${OUT}/ — all valid Swagger 2.0, all < 1 MB.`);
… "1 MB"

Copilot re-review: the generator's flags and summary hard-coded "1 MB" even
though sizeLimitBytes is configurable. Derive a human-readable limitStr from
LIMIT and use it in all three messages (e.g. "< 49 KB" when sizeLimitBytes is
lowered). Proven at 1 MB and at 50 KB.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DXtxaAQHcsDThJ6BaqwXES
@PBNZ
PBNZ requested a review from Copilot July 3, 2026 13:00
@PBNZ

PBNZ commented Jul 3, 2026

Copy link
Copy Markdown
Owner Author

/gemini review

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 15 changed files in this pull request and generated 1 comment.

Comment on lines +158 to +161
let bytes = sizeOf(sw);
if (bytes >= LIMIT) { strip(sw); bytes = sizeOf(sw); } // last-ditch shrink for an oversize def
const file = join(OUT, `${slug(name)}.swagger.json`);
writeFileSync(file, JSON.stringify(sw, null, 2));

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new repository type, power-platform-connectors, to the new-repo skill. This type converts committed Postman collections into valid OpenAPI 2.0 definitions for Microsoft Power Platform, utilizing a pinned Docker toolchain and automated sync workflows. The review feedback highlights several opportunities to improve the robustness of the generation script, including handling process termination signals for temporary directory cleanup, adding defensive checks for malformed Postman collections, and restricting path item iterations to standard HTTP methods. Additionally, it is recommended to run the Docker containers with host user permissions in CI/CD and local environments to prevent file ownership issues.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +20 to +21
const work = mkdtempSync(join(tmpdir(), 'ppc-')); // unique per run; removed on exit
process.on('exit', () => { try { rmSync(work, { recursive: true, force: true }); } catch { /* best-effort */ } });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The 'exit' event in Node.js is not emitted when the process is terminated by signals (like SIGINT or SIGTERM) or due to an uncaught exception. This means the temporary directory created by mkdtempSync will not be cleaned up if the script crashes or is interrupted. Registering handlers for uncaughtException, SIGINT, and SIGTERM that call process.exit() ensures that the cleanup logic always runs.

const work = mkdtempSync(join(tmpdir(), 'ppc-')); // unique per run; removed on exit
process.on('exit', () => { try { rmSync(work, { recursive: true, force: true }); } catch { /* best-effort */ } });
process.on('uncaughtException', (err) => {
  console.error(err);
  process.exit(1);
});
process.on('SIGINT', () => process.exit(130));
process.on('SIGTERM', () => process.exit(143));

Comment on lines +45 to +54
function resolveVars(obj, vars) {
if (typeof obj === 'string') {
let s = obj;
for (const v of vars) if (v && v.key && v.value != null) s = s.split(`{{${v.key}}}`).join(String(v.value));
return s;
}
if (Array.isArray(obj)) return obj.map((x) => resolveVars(x, vars));
if (obj && typeof obj === 'object') { const r = {}; for (const [k, val] of Object.entries(obj)) r[k] = resolveVars(val, vars); return r; }
return obj;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If vars is not an array (e.g., if it is undefined, null, or a string), iterating over it with for...of will throw a TypeError or yield unexpected behavior. Adding a defensive check to ensure vars is treated as an array makes the variable resolution much more robust.

Suggested change
function resolveVars(obj, vars) {
if (typeof obj === 'string') {
let s = obj;
for (const v of vars) if (v && v.key && v.value != null) s = s.split(`{{${v.key}}}`).join(String(v.value));
return s;
}
if (Array.isArray(obj)) return obj.map((x) => resolveVars(x, vars));
if (obj && typeof obj === 'object') { const r = {}; for (const [k, val] of Object.entries(obj)) r[k] = resolveVars(val, vars); return r; }
return obj;
}
function resolveVars(obj, vars) {
const varList = Array.isArray(vars) ? vars : [];
if (typeof obj === 'string') {
let s = obj;
for (const v of varList) if (v && v.key && v.value != null) s = s.split(`{{${v.key}}}`).join(String(v.value));
return s;
}
if (Array.isArray(obj)) return obj.map((x) => resolveVars(x, varList));
if (obj && typeof obj === 'object') { const r = {}; for (const [k, val] of Object.entries(obj)) r[k] = resolveVars(val, varList); return r; }
return obj;
}

Comment on lines +59 to +78
function pmAuthToSwagger(auth) {
if (!auth || !auth.type) return null;
switch (auth.type) {
case 'apikey': {
const a = auth.apikey || [];
const loc = (kv(a, 'in') || 'header').toLowerCase() === 'query' ? 'query' : 'header';
return { name: 'apiKeyAuth', def: { type: 'apiKey', name: kv(a, 'key') || 'Authorization', in: loc } };
}
case 'bearer':
return { name: 'bearerAuth', def: { type: 'apiKey', name: 'Authorization', in: 'header' } };
case 'basic':
return { name: 'basicAuth', def: { type: 'basic' } };
case 'oauth2': {
const a = auth.oauth2 || [];
return { name: 'oauth2Auth', def: { type: 'oauth2', flow: 'accessCode', authorizationUrl: kv(a, 'authUrl') || '', tokenUrl: kv(a, 'accessTokenUrl') || '', scopes: {} } };
}
default:
return null;
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If auth.apikey or auth.oauth2 are not arrays, calling helper functions like kv (which uses .find()) will throw a runtime error. Ensuring they are arrays before processing prevents potential crashes on malformed collections.

Suggested change
function pmAuthToSwagger(auth) {
if (!auth || !auth.type) return null;
switch (auth.type) {
case 'apikey': {
const a = auth.apikey || [];
const loc = (kv(a, 'in') || 'header').toLowerCase() === 'query' ? 'query' : 'header';
return { name: 'apiKeyAuth', def: { type: 'apiKey', name: kv(a, 'key') || 'Authorization', in: loc } };
}
case 'bearer':
return { name: 'bearerAuth', def: { type: 'apiKey', name: 'Authorization', in: 'header' } };
case 'basic':
return { name: 'basicAuth', def: { type: 'basic' } };
case 'oauth2': {
const a = auth.oauth2 || [];
return { name: 'oauth2Auth', def: { type: 'oauth2', flow: 'accessCode', authorizationUrl: kv(a, 'authUrl') || '', tokenUrl: kv(a, 'accessTokenUrl') || '', scopes: {} } };
}
default:
return null;
}
}
function pmAuthToSwagger(auth) {
if (!auth || !auth.type) return null;
switch (auth.type) {
case 'apikey': {
const a = Array.isArray(auth.apikey) ? auth.apikey : [];
const loc = (kv(a, 'in') || 'header').toLowerCase() === 'query' ? 'query' : 'header';
return { name: 'apiKeyAuth', def: { type: 'apiKey', name: kv(a, 'key') || 'Authorization', in: loc } };
}
case 'bearer':
return { name: 'bearerAuth', def: { type: 'apiKey', name: 'Authorization', in: 'header' } };
case 'basic':
return { name: 'basicAuth', def: { type: 'basic' } };
case 'oauth2': {
const a = Array.isArray(auth.oauth2) ? auth.oauth2 : [];
return { name: 'oauth2Auth', def: { type: 'oauth2', flow: 'accessCode', authorizationUrl: kv(a, 'authUrl') || '', tokenUrl: kv(a, 'accessTokenUrl') || '', scopes: {} } };
}
default:
return null;
}
}

Comment on lines +84 to +90
function fixResponses(sw) {
for (const path of Object.values(sw.paths || {}))
for (const op of Object.values(path))
if (op && typeof op === 'object' && op.responses)
for (const [code, r] of Object.entries(op.responses))
if (r && typeof r === 'object' && !r.description) r.description = REASON[code] || 'Response';
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Iterating over all properties of a path item using Object.values(path) can accidentally process non-operation properties (such as parameters arrays, $ref, or vendor extensions like x-*). Restricting the iteration to the standard HTTP methods ensures that only actual operation objects are modified.

function fixResponses(sw) {
  const methods = ['get', 'put', 'post', 'delete', 'options', 'head', 'patch'];
  for (const path of Object.values(sw.paths || {})) {
    for (const m of methods) {
      const op = path[m];
      if (op && typeof op === 'object' && op.responses) {
        for (const [code, r] of Object.entries(op.responses)) {
          if (r && typeof r === 'object' && !r.description) r.description = REASON[code] || 'Response';
        }
      }
    }
  }
}

Comment on lines +94 to +113
function fixPaths(sw) {
if (!sw.paths) return;
const fixed = {};
for (const [key, item] of Object.entries(sw.paths)) {
const pathKey = key.startsWith('/') ? key : '/' + key;
const tokens = [...pathKey.matchAll(/\{([^}]+)\}/g)].map((m) => m[1]);
if (tokens.length && item && typeof item === 'object') {
for (const op of Object.values(item)) {
if (op && typeof op === 'object' && op.responses) {
op.parameters = op.parameters || [];
for (const t of tokens)
if (!op.parameters.some((p) => p.in === 'path' && p.name === t))
op.parameters.push({ name: t, in: 'path', required: true, type: 'string' });
}
}
}
fixed[pathKey] = item;
}
sw.paths = fixed;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to fixResponses, iterating over all properties of a path item using Object.values(item) can process non-operation properties like the path-level parameters array. Restricting the iteration to standard HTTP methods is safer and more robust.

function fixPaths(sw) {
  if (!sw.paths) return;
  const methods = ['get', 'put', 'post', 'delete', 'options', 'head', 'patch'];
  const fixed = {};
  for (const [key, item] of Object.entries(sw.paths)) {
    const pathKey = key.startsWith('/') ? key : '/' + key;
    const tokens = [...pathKey.matchAll(/\{([^}]+)\}/g)].map((m) => m[1]);
    if (tokens.length && item && typeof item === 'object') {
      for (const m of methods) {
        const op = item[m];
        if (op && typeof op === 'object' && op.responses) {
          op.parameters = op.parameters || [];
          for (const t of tokens) {
            if (!op.parameters.some((p) => p.in === 'path' && p.name === t)) {
              op.parameters.push({ name: t, in: 'path', required: true, type: 'string' });
            }
          }
        }
      }
    }
    fixed[pathKey] = item;
  }
  sw.paths = fixed;
}

Comment on lines +173 to +186
const roots = collection.item || [];
for (const folder of roots.filter((it) => it.item)) {
const sub = {
info: { ...collection.info, name: `${collection.info?.name || ''} - ${folder.name}`.trim() },
variable: rootVars,
item: folder.item,
};
emit(folder.name, convert(sub, slug(folder.name), folder.auth || rootAuth));
}
const loose = roots.filter((it) => it.request);
if (loose.length) {
const sub = { info: { ...collection.info, name: `${collection.info?.name || ''} - misc` }, variable: rootVars, item: loose };
emit('misc', convert(sub, 'misc', rootAuth));
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If roots contains null or undefined elements, or if a folder does not have a name property, the script will throw a runtime error. Adding defensive checks and fallback values prevents potential crashes.

Suggested change
const roots = collection.item || [];
for (const folder of roots.filter((it) => it.item)) {
const sub = {
info: { ...collection.info, name: `${collection.info?.name || ''} - ${folder.name}`.trim() },
variable: rootVars,
item: folder.item,
};
emit(folder.name, convert(sub, slug(folder.name), folder.auth || rootAuth));
}
const loose = roots.filter((it) => it.request);
if (loose.length) {
const sub = { info: { ...collection.info, name: `${collection.info?.name || ''} - misc` }, variable: rootVars, item: loose };
emit('misc', convert(sub, 'misc', rootAuth));
}
const roots = collection.item || [];
for (const folder of roots.filter((it) => it && it.item)) {
const sub = {
info: { ...collection.info, name: `${collection.info?.name || ''} - ${folder.name || 'folder'}`.trim() },
variable: rootVars,
item: folder.item,
};
emit(folder.name || 'folder', convert(sub, slug(folder.name || 'folder'), folder.auth || rootAuth));
}
const loose = roots.filter((it) => it && it.request);
if (loose.length) {
const sub = { info: { ...collection.info, name: `${collection.info?.name || ''} - misc` }, variable: rootVars, item: loose };
emit('misc', convert(sub, 'misc', rootAuth));
}

Comment on lines +22 to +23
- name: Generate + self-validate (fails on invalid Swagger 2.0 or >= 1 MB)
run: docker run --rm -v "${{ github.workspace }}:/work" -w /work ppc-gen node scripts/generate.mjs

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Running the Docker container as root can result in generated files in the workspace being owned by root. This can cause permission issues in subsequent steps or local environments. Passing --user "$(id -u):$(id -g)" ensures files are created with the correct host user ownership.

      - name: Generate + self-validate (fails on invalid Swagger 2.0 or >= 1 MB)
        run: docker run --rm --user "$(id -u):$(id -g)" -v "${{ github.workspace }}:/work" -w /work ppc-gen node scripts/generate.mjs

Comment on lines +65 to +67
run: |
docker build -t ppc-gen .
docker run --rm -v "${{ github.workspace }}:/work" -w /work ppc-gen node scripts/generate.mjs

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Running the Docker container as root can result in generated files in the workspace being owned by root, which can cause permission issues in subsequent steps (such as git add or git commit). Passing --user "$(id -u):$(id -g)" ensures files are created with the correct host user ownership.

        run: |
          docker build -t ppc-gen .
          docker run --rm --user "$(id -u):$(id -g)" -v "${{ github.workspace }}:/work" -w /work ppc-gen node scripts/generate.mjs


```sh
docker build -t {{name}}-gen .
docker run --rm -v "${PWD}:/work" {{name}}-gen node scripts/generate.mjs

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Running the Docker container as root locally can result in generated files in the workspace being owned by root, which prevents local developers from editing or deleting them without using sudo. Passing --user "$(id -u):$(id -g)" ensures files are created with the correct host user ownership.

   docker run --rm --user "$(id -u):$(id -g)" -v "${PWD}:/work" {{name}}-gen node scripts/generate.mjs

…t docker

Addresses the second Copilot + Gemini re-review:
- generate.mjs: clean the temp dir on SIGINT/SIGTERM too (not just normal exit);
  restrict fixResponses/fixPaths to real HTTP methods (skip path-level
  parameters/$ref/x-* extensions); disambiguate output filenames that slug to the
  same name (uniqueSlug); guard against null items / missing folder names.
- ci.yml + sync.yml: run the container with --user "$(id -u):$(id -g)" so
  generated files aren't root-owned (avoids permission issues before git add);
  de-hard-code "1 MB" in the CI step name.
- README: note the --user tip for Linux/macOS (kept out of the command so the
  Windows/PowerShell copy-paste still works).

Re-proven: USPS -> valid; 50 KB limit -> 25 valid split files; runs correctly as
a non-root user.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DXtxaAQHcsDThJ6BaqwXES
@PBNZ
PBNZ requested a review from Copilot July 3, 2026 13:13
@PBNZ

PBNZ commented Jul 3, 2026

Copy link
Copy Markdown
Owner Author

/gemini review

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 15 changed files in this pull request and generated 2 comments.

Comment on lines +73 to +76
case 'oauth2': {
const a = auth.oauth2 || [];
return { name: 'oauth2Auth', def: { type: 'oauth2', flow: 'accessCode', authorizationUrl: kv(a, 'authUrl') || '', tokenUrl: kv(a, 'accessTokenUrl') || '', scopes: {} } };
}
Comment on lines +36 to +41
# Send the Postman API key only when hitting the Postman API.
AUTH=()
case "$URL" in *api.getpostman.com*)
[ -n "${POSTMAN_API_KEY:-}" ] && AUTH=(-H "X-Api-Key: ${POSTMAN_API_KEY}");;
esac
curl -fsSL "${AUTH[@]}" "$URL" -o /tmp/fetched.json

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the power-platform-connectors repository type, which converts a committed Postman collection into valid OpenAPI 2.0 definitions for Microsoft Power Platform custom connectors using a pinned Docker toolchain. It includes a generator script, CI/CD workflows, and comprehensive documentation. The review feedback highlights critical improvements for the generator script, such as cleaning up operation-level security blocks to prevent unresolved references, ensuring sub-collections retain their authentication context during splitting, and safely handling missing manifest files in the sync workflow to avoid pipeline crashes.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +125 to +135
function fixSecurity(sw, auth) {
const s = pmAuthToSwagger(auth);
if (s) {
sw.securityDefinitions = { [s.name]: s.def };
sw.security = [{ [s.name]: [] }];
} else {
if (auth?.type) warnings.push(`auth type "${auth.type}" not mapped — add security manually in the connector`);
delete sw.securityDefinitions; // never ship an invalid securityDefinitions block
delete sw.security;
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

When overriding the global securityDefinitions and security at the root level, any existing operation-level security blocks generated by the converter (which refer to the old, deleted security definitions) will become unresolved references. This can cause validation failures in strict Swagger 2.0 parsers.

To prevent this, we should clean up any operation-level security arrays so that they automatically fall back to the global security definition we defined.

function fixSecurity(sw, auth) {
  const s = pmAuthToSwagger(auth);
  if (s) {
    sw.securityDefinitions = { [s.name]: s.def };
    sw.security = [{ [s.name]: [] }];
  } else {
    if (auth?.type) warnings.push("auth type \"" + auth.type + "\" not mapped — add security manually in the connector");
    delete sw.securityDefinitions; // never ship an invalid securityDefinitions block
    delete sw.security;
  }
  // Clean up operation-level security to avoid unresolved references to deleted definitions
  if (sw.paths) {
    for (const path of Object.values(sw.paths)) {
      if (path && typeof path === 'object') {
        for (const m of METHODS) {
          if (path[m] && typeof path[m] === 'object') {
            delete path[m].security;
          }
        }
      }
    }
  }
}

Comment on lines +191 to +195
const sub = {
info: { ...collection.info, name: `${collection.info?.name || ''} - ${fname}`.trim() },
variable: rootVars,
item: folder.item,
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When splitting the collection per top-level folder, the sub-collection object sub is passed to convert(). However, the auth block is not included in the sub object itself.

While convert() receives folder.auth || rootAuth as effectiveAuth to manually patch the final Swagger 2.0 output, postman-to-openapi (p2o) runs on the sub collection without any auth context. This can lead to loss of auth-related metadata or incorrect conversion behavior during the initial OpenAPI 3.0 generation phase. Including the resolved auth block in the sub-collection ensures p2o processes it with the correct context.

Suggested change
const sub = {
info: { ...collection.info, name: `${collection.info?.name || ''} - ${fname}`.trim() },
variable: rootVars,
item: folder.item,
};
const sub = {
info: { ...collection.info, name: ((collection.info?.name || '') + ' - ' + fname).trim() },
variable: rootVars,
auth: folder.auth || rootAuth,
item: folder.item,
};

}
const loose = roots.filter((it) => it && it.request);
if (loose.length) {
const sub = { info: { ...collection.info, name: `${collection.info?.name || ''} - misc` }, variable: rootVars, item: loose };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similarly to the folder sub-collections, the misc sub-collection should also include the auth block so that postman-to-openapi (p2o) has the correct auth context during conversion.

Suggested change
const sub = { info: { ...collection.info, name: `${collection.info?.name || ''} - misc` }, variable: rootVars, item: loose };
const sub = { info: { ...collection.info, name: ((collection.info?.name || '') + ' - misc').trim() }, variable: rootVars, auth: rootAuth, item: loose };

cp /tmp/fetched.json /tmp/coll.json
fi
NEW=$(jq -S 'del(.info._postman_id)' /tmp/coll.json | sha256sum | cut -d' ' -f1)
OLD=$(jq -r '.sha256 // ""' .postman/manifest.json)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If .postman/manifest.json is missing or deleted, the jq command will fail with a "No such file or directory" error. Since set -euo pipefail is active, this will cause the entire workflow step to crash.

Checking if the file exists before running jq ensures the workflow runs robustly even if the manifest file is absent.

          OLD=""
          if [ -f .postman/manifest.json ]; then
            OLD=$(jq -r '.sha256 // ""' .postman/manifest.json)
          fi

…ngling security

Two real Swagger 2.0 validity bugs caught by the re-review (USPS uses apikey, so
earlier tests missed them):
- pmAuthToSwagger: an oauth2 block with missing authorizationUrl/tokenUrl produced
  an invalid def (accessCode flow requires both URLs). Now returns null in that
  case -> no security def + a warning, so the output stays valid.
- fixSecurity: overriding the root securityDefinitions left operation-level
  `security` blocks pointing at the deleted scheme (a dangling ref). Now strip
  operation-level security so only the single root-level definition applies.

Also (hardening): sync.yml only sends the Postman API key to https://api.getpostman.com/
(not look-alike hosts), adds curl --max-time/--retry, and tolerates a missing
manifest; split sub-collections carry auth for p2o context.

Proven: oauth2 with URLs -> valid oauth2 def; without -> valid (no security);
USPS apikey regression still valid.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DXtxaAQHcsDThJ6BaqwXES
@PBNZ
PBNZ requested a review from Copilot July 3, 2026 13:24
@PBNZ

PBNZ commented Jul 3, 2026

Copy link
Copy Markdown
Owner Author

/gemini review

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 15 changed files in this pull request and generated 2 comments.

Comment on lines +95 to +97
if (op && typeof op === 'object' && op.responses)
for (const [code, r] of Object.entries(op.responses))
if (r && typeof r === 'object' && !r.description) r.description = REASON[code] || 'Response';
Comment on lines +80 to +83
git checkout -B "$BR"
git add -A
git commit -m "chore: sync connector definitions from upstream Postman collection"
git push -f -u origin "$BR"

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new power-platform-connectors repository type that converts a committed Postman collection into OpenAPI 2.0 custom-connector definitions for Microsoft Power Platform, complete with templates, a pinned Docker toolchain, and CI/sync workflows. Feedback on the generator script focuses on addressing a potential filename collision bug in slug generation, ensuring temporary directories are cleaned up on uncaught exceptions or unhandled rejections, and optimizing the variable resolution logic to prevent excessive string allocations.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +175 to +181
const usedSlugs = new Map();
function uniqueSlug(name) {
const base = slug(name);
const n = (usedSlugs.get(base) || 0) + 1;
usedSlugs.set(base, n);
return n === 1 ? base : `${base}-${n}`; // disambiguate folders that slug to the same name
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There is a potential filename collision bug in uniqueSlug.

If the collection contains folders that slug to names that conflict with existing numbered suffixes (for example, a folder named "My Folder" and another folder named "My Folder 2"), they will both resolve to the same slug "my-folder-2". The second one processed will overwrite the first one's output file.

Using a Set to track all allocated slugs and incrementing the suffix until a unique one is found guarantees that no collisions or overwrites occur.

const allocatedSlugs = new Set();
function uniqueSlug(name) {
  const base = slug(name);
  let candidate = base;
  let n = 1;
  while (allocatedSlugs.has(candidate)) {
    n++;
    candidate = base + '-' + n;
  }
  allocatedSlugs.add(candidate);
  return candidate;
}

Comment on lines +21 to +23
const cleanup = () => { try { rmSync(work, { recursive: true, force: true }); } catch { /* best-effort */ } };
process.on('exit', cleanup);
for (const sig of ['SIGINT', 'SIGTERM']) process.on(sig, () => { cleanup(); process.exit(1); });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The temporary directory created by mkdtempSync is currently only cleaned up on normal exit (process.on('exit')) or on SIGINT/SIGTERM. If the script crashes due to an uncaught exception or an unhandled promise rejection (for example, if execFileSync fails), the temporary directory will be leaked in the system's temp folder.

Adding handlers for uncaughtException and unhandledRejection ensures that the temporary directory is always cleaned up on failure.

Suggested change
const cleanup = () => { try { rmSync(work, { recursive: true, force: true }); } catch { /* best-effort */ } };
process.on('exit', cleanup);
for (const sig of ['SIGINT', 'SIGTERM']) process.on(sig, () => { cleanup(); process.exit(1); });
const cleanup = () => { try { rmSync(work, { recursive: true, force: true }); } catch { /* best-effort */ } };
process.on('exit', cleanup);
for (const sig of ['SIGINT', 'SIGTERM']) process.on(sig, () => { cleanup(); process.exit(1); });
process.on('uncaughtException', (err) => { cleanup(); console.error(err); process.exit(1); });
process.on('unhandledRejection', (reason) => { cleanup(); console.error(reason); process.exit(1); });

Comment on lines +47 to +56
function resolveVars(obj, vars) {
if (typeof obj === 'string') {
let s = obj;
for (const v of vars) if (v && v.key && v.value != null) s = s.split(`{{${v.key}}}`).join(String(v.value));
return s;
}
if (Array.isArray(obj)) return obj.map((x) => resolveVars(x, vars));
if (obj && typeof obj === 'object') { const r = {}; for (const [k, val] of Object.entries(obj)) r[k] = resolveVars(val, vars); return r; }
return obj;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current implementation of resolveVars performs a split().join() operation for every single variable in vars on every string in the collection. For larger collections with many variables and strings, this results in $O(N \times V)$ string allocations and array creations, which can cause significant performance degradation and garbage collection overhead.

We can optimize this to $O(N)$ by building a Map of the variables once, and then using a single regular expression replacement (replace(/{{([^}]+)}}/g, ...)) on each string.

function resolveVars(obj, vars) {
  const varMap = new Map(vars.filter(v => v && v.key && v.value != null).map(v => [v.key, String(v.value)]));
  function recurse(val) {
    if (typeof val === 'string') {
      return val.replace(/{{([^}]+)}}/g, (match, key) => varMap.has(key) ? varMap.get(key) : match);
    }
    if (Array.isArray(val)) return val.map(recurse);
    if (val && typeof val === 'object') {
      const r = {};
      for (const [k, v] of Object.entries(val)) r[k] = recurse(v);
      return r;
    }
    return val;
  }
  return recurse(obj);
}

…g collisions

Two more edge-case validity fixes from the re-review:
- fixResponses: don't add a `description` to a `$ref` response object (in
  Swagger 2.0 a `$ref` must be the only key; a sibling `description` is invalid).
- uniqueSlug: track the actual set of used filenames and increment until free, so
  two "My Folder" folders plus a real "My Folder 2" produce three distinct files
  instead of colliding.

Proven: USPS still valid; duplicate folder names -> 3 distinct outputs.

Deliberately not changed (evaluated, low value): --force-with-lease on the solo
bot-managed sync branch; an O(N*V) micro-opt in resolveVars; an explicit
uncaughtException cleanup handler (Node's 'exit' already fires on that, so the
temp dir is cleaned).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DXtxaAQHcsDThJ6BaqwXES
@PBNZ
PBNZ requested a review from Copilot July 3, 2026 13:33

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 15 changed files in this pull request and generated 1 comment.

Comment on lines +30 to +35
URL=$(jq -r '.sourceUrl // ""' connectors.config.json)
case "$URL" in
*"<fill me"* | "" | null)
echo "sourceUrl is not set in connectors.config.json — skipping."
echo "changed=false" >> "$GITHUB_OUTPUT"; exit 0;;
esac
PBNZ and others added 2 commits July 4, 2026 02:03
…f-review

Fresh critical-review pass (own reviewer agents) on generate.mjs + the
workflows, fixing correctness and robustness gaps that swagger-cli's
validateFormats:false gate would let pass silently:

generate.mjs
- C1: scan each output for residual {{var}} / %7b%7b (unresolved Postman
  *environment* variables) and fail the build — previously shipped a broken
  connector reported as "all valid Swagger 2.0". Verified: exit 2 + clear flag.
- H1: resolve committed {{vars}} inside the auth block before mapping, so OAuth
  URLs / apiKey header names don't leak literally. Verified.
- H2: map every OAuth2 grant to the correct Swagger 2.0 flow (client_credentials
  ->application, password->password, implicit->implicit, auth_code->accessCode),
  requiring only the URL(s) that flow needs (verified vs the OpenAPI 2.0 spec).
  Previously hardcoded accessCode -> shipped an authless connector for the common
  client-credentials case. Verified: client_credentials -> flow:application.
- H3: warn when auth is set only per-request (not mapped) instead of silently
  emitting no securityDefinitions.
- M1: try dropping examples on the whole collection before deciding to split, so
  a barely-over collection stays one file instead of fragmenting.
- M2: per-(sub)collection try/catch so one folder that fails conversion no longer
  aborts the rest of the split; failures are reported and still exit non-zero.
- LOW: guard kv() against v2.0 object-form auth (no more TypeError crash,
  verified), treat noauth as silent no-security, and make resolveVars a bounded
  fixed-point so nested/out-of-order vars resolve.

workflows
- sync H1: --max-redirs 0 on the authed Postman fetch + --proto/--proto-redir
  =https so the API key can't follow a redirect to another host.
- sync H2: open the PR only if none exists; don't mask other gh errors.
- ci/sync: least-privilege permissions on ci.yml, concurrency guard on sync,
  scoped git add, HOME=/tmp for the container, drop the daily-churn timestamp.
- Dockerfile: npm install --ignore-scripts (defence-in-depth; tools verified to
  still run).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DXtxaAQHcsDThJ6BaqwXES
…ector-critical fields

Follow-up to the C1 fix. The whole-document regex scan would false-fail on a
collection that legitimately documents a {{var}} in a description/example. Scope
it to the fields Power Platform actually calls — host, basePath, schemes, path
keys, and the security URLs / apiKey header name — which is both precise and the
exact set swagger-cli can't check (it runs validateFormats:false).

Verified empirically: an unresolved env var in an oauth2 tokenUrl with an
otherwise-valid host is reported *valid* by swagger-cli alone (exit 0) but flagged
by the scoped scan (exit 2, naming oauth2Auth.tokenUrl); a benign {{var}} in a
description now passes (exit 0) instead of false-failing.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DXtxaAQHcsDThJ6BaqwXES
@PBNZ

PBNZ commented Jul 3, 2026

Copy link
Copy Markdown
Owner Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the power-platform-connectors repository type, enabling the conversion of committed Postman collections into OpenAPI 2.0 custom-connector definitions for Microsoft Power Platform. It includes a pinned Docker toolchain, a normalization and splitting generator script, and automated CI/sync workflows. Feedback focuses on improving the reliability of the sync workflow by using gh pr list instead of gh pr view to check for active PRs and supporting EU Postman domains, as well as adding defensive checks in the generator script to prevent runtime crashes on malformed Swagger inputs.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

# Open a PR only if one isn't already open for this branch. Don't swallow other errors
# (e.g. "GitHub Actions is not permitted to create pull requests") — a silently-dead sync
# must fail loudly.
if ! gh pr view "$BR" --json number >/dev/null 2>&1; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Using gh pr view to check if a PR is already open is unreliable because it returns the most recent PR for the branch regardless of its state (including MERGED or CLOSED). This means that once the first sync PR is merged or closed, subsequent runs of the workflow will see the merged/closed PR, assume a PR is already open, and fail to create a new PR for any new upstream changes.

Instead, use gh pr list --head "$BR" to check if there is an active, open PR for the branch.

          if ! gh pr list --head "$BR" --json number | jq -e 'length > 0' >/dev/null 2>&1; then

Comment on lines +45 to +47
case "$URL" in "https://api.getpostman.com/"*)
[ -n "${POSTMAN_API_KEY:-}" ] && AUTH=(--max-redirs 0 -H "X-Api-Key: ${POSTMAN_API_KEY}");;
esac

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current case statement only matches the standard https://api.getpostman.com/ domain. However, EU users on Enterprise plans use https://api.eu.postman.com/, and some users may use https://api.postman.com/.

Updating the pattern to match these alternative domains ensures that the X-Api-Key header is correctly appended for all Postman API endpoints.

          case "$URL" in
            "https://api.getpostman.com/"* | "https://api.postman.com/"* | "https://api.eu.postman.com/"*)
              [ -n "${POSTMAN_API_KEY:-}" ] && AUTH=(--max-redirs 0 -H "X-Api-Key: ${POSTMAN_API_KEY}");;
          esac

if (!path || typeof path !== 'object') continue;
for (const m of METHODS) {
const op = path[m];
if (op && typeof op === 'object' && op.responses)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To prevent runtime errors when processing malformed or unexpected Swagger inputs, add a defensive check to ensure op.responses is a valid object before calling Object.entries() on it.

Suggested change
if (op && typeof op === 'object' && op.responses)
if (op && typeof op === 'object' && op.responses && typeof op.responses === 'object')

for (const m of METHODS) {
const op = item[m];
if (op && typeof op === 'object') {
op.parameters = op.parameters || [];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If op.parameters is defined but is not an array (e.g., an object or string), op.parameters.some will throw a TypeError. Adding a defensive check to ensure op.parameters is an array prevents potential runtime crashes.

Suggested change
op.parameters = op.parameters || [];
if (!Array.isArray(op.parameters)) op.parameters = [];

const chk = (label, val) => { if (typeof val === 'string' && TOKEN.test(val)) hits.push(label); };
chk('host', sw.host);
chk('basePath', sw.basePath);
(sw.schemes || []).forEach((s, i) => chk(`schemes[${i}]`, s));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If sw.schemes is defined but is not an array, calling .forEach on it will throw a TypeError. Adding a defensive check to ensure it is an array prevents potential runtime crashes.

  (Array.isArray(sw.schemes) ? sw.schemes : []).forEach((s, i) => chk(\`schemes[\${i}]\`, s));

…3 MEDIUM)

From the final Gemini review round:

- HIGH (sync.yml): use `gh pr list --head --state open` instead of `gh pr view`
  to decide whether a sync PR already exists. `gh pr view` returns the branch's
  most recent PR even when merged/closed, so after the first sync PR merged the
  workflow would wrongly assume a PR still existed and never open another —
  silently dropping all future upstream changes. (Regression from the previous
  round's PR-guard; now open-only.)
- MEDIUM (sync.yml): also match api.postman.com and api.eu.postman.com (EU data
  residency) when attaching the API key — both verified as official Postman API
  hosts; the trailing-"/" anchor still blocks look-alikes.
- MEDIUM (generate.mjs): Array.isArray guards on op.parameters (fixPaths) and
  sw.schemes (the new unresolved-var scan) so malformed converter output can't
  throw a TypeError; plus a typeof-object guard on op.responses.

Verified: Test A (client_credentials) still exits 0; all 5 repo validators green.
Copilot produced no new feedback this round.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DXtxaAQHcsDThJ6BaqwXES
@PBNZ
PBNZ merged commit a9041f1 into main Jul 3, 2026
1 check passed
@PBNZ
PBNZ deleted the feat/power-platform-connectors branch July 3, 2026 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants