Skip to content

Deduplicate model multiplier parsing helpers across API proxy guards#4293

Merged
lpcox merged 3 commits into
mainfrom
copilot/duplicate-code-issue-fix
Jun 4, 2026
Merged

Deduplicate model multiplier parsing helpers across API proxy guards#4293
lpcox merged 3 commits into
mainfrom
copilot/duplicate-code-issue-fix

Conversation

Copilot AI commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Two guard modules (effective-token-guard and max-model-multiplier-guard) carried identical implementations of parseModelMultipliers and parsePositiveNumber, creating duplicated production parsing logic and split maintenance risk. This change centralizes those helpers in guard-utils and updates both guards to consume the shared exports.

  • Shared guard parsing utilities

    • Added parseModelMultipliers(raw) and parsePositiveNumber(raw) to containers/api-proxy/guards/guard-utils.js
    • Exported both alongside existing parsePositiveInteger
  • Guard deduplication

    • Removed inline helper copies from:
      • containers/api-proxy/guards/effective-token-guard.js
      • containers/api-proxy/guards/max-model-multiplier-guard.js
    • Replaced with imports from ./guard-utils
  • Utility coverage

    • Extended containers/api-proxy/guards/guard-utils.test.js with focused cases for:
      • invalid/empty JSON and non-object inputs for model multipliers
      • filtering to finite positive multipliers only
      • positive-number parsing semantics used by both guards
// guard-utils.js
module.exports = {
  parsePositiveInteger,
  parseModelMultipliers,
  parsePositiveNumber,
};

// effective-token-guard.js / max-model-multiplier-guard.js
const { parseModelMultipliers, parsePositiveNumber } = require('./guard-utils');

Copilot AI changed the title [WIP] Refactor duplicated parsing functions in guard modules Deduplicate model multiplier parsing helpers across API proxy guards Jun 3, 2026
Copilot finished work on behalf of lpcox June 3, 2026 23:15
Copilot AI requested a review from lpcox June 3, 2026 23:15
@lpcox lpcox marked this pull request as ready for review June 3, 2026 23:55
Copilot AI review requested due to automatic review settings June 3, 2026 23:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR reduces duplicated parsing logic in the API proxy guard layer by centralizing shared “model multiplier” parsing helpers into guard-utils, and updating both effective-token-guard and max-model-multiplier-guard to use the shared implementation.

Changes:

  • Added parseModelMultipliers(raw) and parsePositiveNumber(raw) to containers/api-proxy/guards/guard-utils.js and exported them alongside parsePositiveInteger.
  • Removed the duplicated helper implementations from effective-token-guard.js and max-model-multiplier-guard.js, replacing them with imports from ./guard-utils.
  • Expanded guard-utils.test.js to cover invalid inputs and expected filtering/semantics for both new helpers.
Show a summary per file
File Description
containers/api-proxy/guards/guard-utils.js Centralizes parseModelMultipliers and parsePositiveNumber in the shared guard utilities module.
containers/api-proxy/guards/guard-utils.test.js Adds unit coverage for the newly centralized parsing helpers, including invalid and edge inputs.
containers/api-proxy/guards/effective-token-guard.js Switches from inline helper copies to importing the shared parsing helpers from guard-utils.
containers/api-proxy/guards/max-model-multiplier-guard.js Switches from inline helper copies to importing the shared parsing helpers from guard-utils.

Copilot's findings

Tip

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

  • Files reviewed: 4/4 changed files
  • Comments generated: 0

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

✅ Coverage Check Passed

Overall Coverage

Metric Base PR Delta
Lines 96.53% 96.57% 📈 +0.04%
Statements 96.41% 96.45% 📈 +0.04%
Functions 98.69% 98.69% ➡️ +0.00%
Branches 90.53% 90.56% 📈 +0.03%
📁 Per-file Coverage Changes (1 files)
File Lines (Before → After) Statements (Before → After)
src/config-writer.ts 89.3% → 90.9% (+1.65%) 89.3% → 90.9% (+1.65%)

Coverage comparison generated by scripts/ci/compare-coverage.ts

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions github-actions Bot mentioned this pull request Jun 3, 2026
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@lpcox

lpcox commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

Copilot AI commented Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

@copilot fix this failing ci check https://github.com/github/gh-aw-firewall/actions/runs/26919029512/job/79420287272?pr=4293

Fixed in commit a17e096 by making the flaky blocked-domain integration case retry transient curl failures (--retry 3 --retry-all-errors --retry-delay 1) when checking https://github.com. This addresses the 502-based CI flake in Domain Tests.

Copilot finished work on behalf of lpcox June 4, 2026 00:36
@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Smoke Test: Claude Engine

  • ✅ GitHub API: 2 PR entries found
  • ✅ GitHub check: playwright_check=PASS
  • ✅ File verify: smoke-test file exists

Total: PASS

💥 [THE END] — Illustrated by Smoke Claude

@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

🔍 Smoke Test Results

Test Result
GitHub MCP connectivity
GitHub.com HTTP connectivity ⚠️ (pre-step data unavailable — template vars not expanded)
File write/read ⚠️ (pre-step data unavailable — template vars not expanded)

PR: Deduplicate model multiplier parsing helpers across API proxy guards
Author: @Copilot | Assignees: @lpcox, @Copilot

Overall: PARTIAL — MCP ✅, pre-step outputs were not expanded so HTTP/file tests could not be verified.

📰 BREAKING: Report filed by Smoke Copilot

@github-actions

This comment has been minimized.

@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

🔭 Smoke Test: API Proxy OpenTelemetry Tracing

Scenario Result Notes
1. Module Loading ✅ Pass otel.js loads cleanly; exports: startRequestSpan, setTokenAttributes, endSpan, endSpanError, shutdown, isEnabled + internal helpers
2. Test Suite ✅ Pass 33/33 tests passed across span creation, token attributes, error handling, OTLP serialization, proxy exporter, file exporter
3. Env Var Forwarding ✅ Pass src/services/api-proxy-service-config.ts forwards OTEL_EXPORTER_OTLP_ENDPOINT, OTEL_EXPORTER_OTLP_HEADERS, GITHUB_AW_OTEL_TRACE_ID, GITHUB_AW_OTEL_PARENT_SPAN_ID, and all OTEL_* vars to the api-proxy container
4. Token Tracker Integration ✅ Pass onUsage callback exists in token-tracker-http.js and is invoked with normalized usage — OTEL hook point is wired
5. OTEL Diagnostics ✅ Pass No OTLP endpoint configured in this run (graceful degradation path); file fallback to /var/log/api-proxy/otel.jsonl is the expected behavior

All 5 scenarios pass. OTEL tracing integration is complete and functional.

📡 OTel tracing validated by Smoke OTel Tracing

@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Smoke test result: FAIL

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • localhost

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "localhost"

See Network Configuration for more information.

💎 Faceted by Smoke Gemini

@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Deduplicate model multiplier parsing helpers across API proxy guards
fix(api-proxy): use api-key header for Azure OpenAI BYOK requests
✅ GitHub PR query
✅ Browser title check
✅ File write/read
✅ Discussion comment
✅ Build
Overall: PASS

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • registry.npmjs.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "registry.npmjs.org"

See Network Configuration for more information.

🔮 The oracle has spoken through Smoke Codex

@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Chroot Version Comparison Results

Runtime Host Version Chroot Version Match?
Python Python 3.12.13 Python 3.12.3 ❌ NO
Node.js v24.16.0 v22.22.3 ❌ NO
Go go1.22.12 go1.22.12 ✅ YES

Overall: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot.

Tested by Smoke Chroot

@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

🏗️ Build Test Suite Results

Ecosystem Project Build/Install Tests Status
Bun elysia 1/1 passed ✅ PASS
Bun hono 1/1 passed ✅ PASS
C++ fmt N/A ✅ PASS
C++ json N/A ✅ PASS
Deno oak N/A 1/1 passed ✅ PASS
Deno std N/A 1/1 passed ✅ PASS
.NET hello-world N/A ✅ PASS
.NET json-parse N/A ✅ PASS
Go color 1/1 passed ✅ PASS
Go env 1/1 passed ✅ PASS
Go uuid 1/1 passed ✅ PASS
Java gson 1/1 passed ✅ PASS
Java caffeine 1/1 passed ✅ PASS
Node.js clsx passed ✅ PASS
Node.js execa passed ✅ PASS
Node.js p-limit passed ✅ PASS
Rust fd 1/1 passed ✅ PASS
Rust zoxide 1/1 passed ✅ PASS

Overall: 8/8 ecosystems passed — PASS 🎉

Generated by Build Test Suite for issue #4293 · sonnet46 913.9K ·

@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Smoke Test: GitHub Actions Services Connectivity

Check Result
Redis PING ❌ Timeout/no response
PostgreSQL pg_isready no response
PostgreSQL SELECT 1 ❌ Not attempted (pg_isready failed)

Overall: FAILhost.docker.internal is unreachable from this environment. Service containers may not be running or the host alias is not resolvable.

🔌 Service connectivity validated by Smoke Services

@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Smoke Test: Copilot BYOK (Direct) — PASS

Test Result
GitHub MCP connectivity ✅ PR #4293 retrieved
GitHub.com HTTP ✅ 200
File write/read /tmp/gh-aw/agent/smoke-test-copilot-byok-26924452467.txt verified
BYOK inference (api-proxy → api.githubcopilot.com)

Running in direct BYOK mode (COPILOT_PROVIDER_API_KEY) via api-proxy → api.githubcopilot.com. PR by @Copilot, assignees: @lpcox @Copilot. Overall: PASS

🔑 BYOK report filed by Smoke Copilot BYOK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Duplicate Code] parseModelMultipliers and parsePositiveNumber duplicated verbatim in two guard modules

3 participants