Python Policy Version Support#1736
Merged
renuka-fernando merged 19 commits intowso2:python-policy-enginefrom Apr 27, 2026
Merged
Python Policy Version Support#1736renuka-fernando merged 19 commits intowso2:python-policy-enginefrom
renuka-fernando merged 19 commits intowso2:python-policy-enginefrom
Conversation
Add a new prompt-compressor policy (sample implementation, policy-definition, requirements, and __init__) plus a default policy and a Python executor unit test. Include the policy in build manifests (filePath and version v0.1.0). Bump versions for multiple existing policies in build-manifest.yaml/build.yaml, add a Postman collection for the gateway API, and apply small updates to generated API code, the gateway runtime Dockerfile, and Makefile.
Add support for direct pip specifications (git+, VCS and PEP 508 "name @ url" forms) in the gateway builder. Introduces sanitizePipSpec, isDirectPipSpec and fetchDirectPipPackage and wires detection into FetchPipPackage so VCS/file/URL-based packages are fetched via `pip wheel --no-deps` and extracted. Error messages and logs sanitize credentials before emitting them. Adds unit tests for direct-spec detection and a git-backed package download test. Update python-deps Dockerfile stage to install git so ephemeral builds can fetch VCS packages. Add a new sample policy package prompt-compressor (pyproject.toml, README, and __version__ in __init__.py).
Switch prompt-compressor packaging from a local filePath to a pipPackage pointing at the git repo (prompt-compressor-v0.1.0) in gateway/build.yaml and gateway/build-manifest.yaml. Bump subscription-validation policy version v1.0.1 -> v1.0.2 in build-manifest.yaml. Update gateway/gateway-runtime/Dockerfile to upgrade pip, setuptools and wheel before installing requirements so PEP 621 pyproject.toml and recent git-installed packages are supported.
Add robust pip package discovery: parse indexed and VCS pip specs into structured refs, support major-only version specifiers (~=major.0) and resolve them to exact tags for VCS refs, sanitize git URLs (remove credentials) and handle direct pip targets. Split fetching into indexed and VCS flows, add runPipCommand helper, read resolved version from wheel METADATA, and preserve the resolved exact pip spec for lockfile generation. Update generator to record resolved PipSpec into build manifest when available, add many unit tests (including VCS resolution and wheel METADATA reading), and tweak build.yaml to point the sample policy at the updated repo ref.
When multiple Python candidates share the same policy name, prefer a discovered pip package whose OriginalPipSpec exactly matches the manifest entry. Adds OriginalPipSpec to DiscoveredPolicy and sets it during discovery, updates the generator to first try matching OriginalPipSpec for python pip packages and fall back to any python candidate if no exact spec match is found. Includes a unit test to verify selecting the correct pip candidate and updates build/build-manifest entries for the prompt-compressor pip specs/versions.
Add support for Go-style short pip URLs and minor-only version resolution for Python policies. Implements short-URL expansion (e.g. github.com/org/repo/path@v1 -> full git+ URL with subdirectory and tag), generalized VCS ref classification (exact / minor-only / major-only), and resolution of partial refs via git ls-remote. Update indexed package parsing to accept ~=N.M.0 (minor-only) and rename the range flag to IsVersionRange. Tests and generator expectations updated; build.yaml and build-manifest.yaml examples adjusted. Also add feature-implementation-plan.md documenting design and verification plan.
Prompt compress
Delete Python sample/default policy files (prompt-compressor and python-* sample policies) and remove the feature implementation plan document. Update gateway/build.yaml to remove the prompt-compressor pip entry and adjust gateway/build-manifest.yaml policy versions (several v1.x.y entries downgraded to earlier patch versions). Also include a small doc comment tweak in generated API file (annotations example). These changes clean up sample policy artifacts and keep manifests consistent with the current packaged policies.
Rename the Python SDK package from wso2_gateway_policy_sdk to apip_sdk_core and update generator, tests and copy paths accordingly. Update sdk-python package layout (files renamed/moved) and adjust python-executor tests to expect apip_sdk_core. Bump multiple policy versions in gateway/build-manifest.yaml. Remove the large gateway-controller Postman collection (postman_collection.json). Other related python-executor and runtime test updates applied to reflect the SDK rename.
sehan-dissanayake
changed the title
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request rebrands the Python SDK from 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
gateway/gateway-runtime/Dockerfile (1)
142-143: Consider pinning pip, setuptools, and wheel versions or use a constraints file for reproducibility.No version pins or constraints are currently applied to the
pip install --upgradecommand on line 143. While the requirements.txt is pre-constrained via the policy-compiler stage, the packaging toolchain versions themselves will float with the base image's default versions at build time. If reproducibility across builds is important, pin these versions explicitly (e.g.,pip3 install --no-cache-dir pip==X.Y.Z setuptools==A.B.C wheel==M.N.O) or apply a constraints file. Note that the upgrade is documented as necessary for PEP 621 and git dependencies support—if pinning, ensure the versions selected maintain this compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-runtime/Dockerfile` around lines 142 - 143, The RUN step upgrading the packaging toolchain (the `pip3 install --no-cache-dir --upgrade pip setuptools wheel` command) should be made reproducible by pinning explicit versions or using a constraints file; update that RUN command to either install pinned versions (e.g., pip==X.Y.Z setuptools==A.B.C wheel==M.N.O) that are known to support PEP 621/git deps or reference a constraints file (e.g., --constraint /path/to/constraints.txt) and document/choose versions that preserve PEP 621 and git dependency support.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gateway/gateway-builder/internal/buildfile/generator.go`:
- Around line 186-200: The code currently falls back to any Python candidate
when me.PipPackage is set, which can pick non-pip policies and leave pipPackage
unresolved; modify the selection in the me.PipPackage branch so the first loop
checks c.Runtime == "python" && c.IsPipPackage && c.OriginalPipSpec ==
me.PipPackage (keep as-is), but change the fallback loop to only consider
candidates where c.Runtime == "python" && c.IsPipPackage (i.e., restrict
fallback to IsPipPackage), and if none match, return/emit an ambiguity or
not-found error instead of selecting a non-pip candidate or leaving pipPackage
unresolved; update any code that uses found after this point to handle the error
path.
In `@gateway/gateway-builder/internal/discovery/python_module.go`:
- Around line 87-91: The code currently treats the last '@' as a git ref
delimiter even when that '@' is part of credentials in the URL; update both
sanitizeURL handling and parseVCSPipSpec to only split on the '@' when the '@'
occurs after the repository path (e.g., after the last '/' of the
authority+path), not merely after the "git+" prefix. Concretely, in the git+
branch where you compute atIdx, ensure atIdx > lastSlashIndex (or similar check
that the '@' is located after the repo path) before using target[:atIdx] and
target[atIdx:]; apply the identical delimiter-position guard in parseVCSPipSpec
and the other occurrence around lines 517-524 so credential-containing URLs are
not mis-split and logs remain safe.
- Around line 262-276: The exactPipSpec currently drops the original index URL
(it builds "%s==%s"), so when resolving refs with an index (ref.IndexURL) the
generated PipPackageInfo.PipSpec loses that info; update the construction of
exactPipSpec to include the index when ref.IndexURL is non-empty (e.g., append
the index using the same "@<index>" form or another canonical pip requirement
form), and ensure the PipPackageInfo returned (PipPackageInfo.PipSpec) uses this
updated exactPipSpec so the resolved manifest preserves the index URL; adjust
the logic around exactPipSpec, ref.IndexURL and the returned PipPackageInfo
accordingly.
- Around line 98-113: isDirectPipSpec currently only recognizes git+ VCS refs
but PEP 508 "pkg @ ..." refs (file://, http(s), etc.) slip through to
expandShortURL and cause confusing errors; update FetchPipPackage to detect any
PEP 508 direct reference (e.g., contains " @ " or matches the "name @ target"
pattern) and if isDirectPipSpec(ref) is false return a clear error saying only
git+ VCS direct references are supported (rather than calling expandShortURL),
or alternatively extend isDirectPipSpec to explicitly recognize non-git PEP 508
targets and surface a similar unsupported error; reference isDirectPipSpec,
FetchPipPackage and expandShortURL when making the change.
In `@gateway/gateway-builder/pkg/types/policy.go`:
- Line 43: Update the comment on the PythonSourceDir field in types/policy.go to
reflect that it is set for both local filePath policies and for extracted pip
policies; locate the PythonSourceDir declaration and change the comment text to
mention "local filePath policies and extracted pip (wheel/egg) policies" or
similar wording so it accurately describes all sources that populate this field.
In `@gateway/gateway-runtime/Dockerfile`:
- Around line 132-138: The build can break if a git+ssh VCS spec is provided
because the Python deps stage lacks SSH client support; either (A) update
parseVCSPipSpec to validate/normalize the incoming VCS specs to only accept
git+https:// (e.g., reject or rewrite git+ssh:// and other schemes) so SSH URLs
cannot reach the python-deps stage, or (B) add openssh-client to the apt-get
install list in the Dockerfile RUN that provisions the python-deps builder stage
so pip can clone ssh:// repos; choose one approach and implement it (reference
parseVCSPipSpec for source validation or the RUN apt-get install line in the
Dockerfile for adding openssh-client).
---
Nitpick comments:
In `@gateway/gateway-runtime/Dockerfile`:
- Around line 142-143: The RUN step upgrading the packaging toolchain (the `pip3
install --no-cache-dir --upgrade pip setuptools wheel` command) should be made
reproducible by pinning explicit versions or using a constraints file; update
that RUN command to either install pinned versions (e.g., pip==X.Y.Z
setuptools==A.B.C wheel==M.N.O) that are known to support PEP 621/git deps or
reference a constraints file (e.g., --constraint /path/to/constraints.txt) and
document/choose versions that preserve PEP 621 and git dependency support.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3eca1068-518d-4bb1-8fad-84e5f0710efe
📒 Files selected for processing (36)
gateway/build-manifest.yamlgateway/build.yamlgateway/gateway-builder/internal/buildfile/generator.gogateway/gateway-builder/internal/buildfile/generator_test.gogateway/gateway-builder/internal/discovery/discovery_test.gogateway/gateway-builder/internal/discovery/manifest.gogateway/gateway-builder/internal/discovery/python_module.gogateway/gateway-builder/internal/discovery/python_module_test.gogateway/gateway-builder/internal/policyengine/generator.gogateway/gateway-builder/internal/policyengine/policyengine_test.gogateway/gateway-builder/pkg/types/policy.gogateway/gateway-controller/default-policies/python-body-buffer-modifier.yamlgateway/gateway-controller/default-policies/python-header-modifier.yamlgateway/gateway-controller/default-policies/python-streaming-logger.yamlgateway/gateway-controller/pkg/api/management/generated.gogateway/gateway-runtime/Dockerfilegateway/gateway-runtime/python-executor/executor/__init__.pygateway/gateway-runtime/python-executor/executor/execution_tracker.pygateway/gateway-runtime/python-executor/executor/instance_store.pygateway/gateway-runtime/python-executor/executor/policy_loader.pygateway/gateway-runtime/python-executor/executor/server.pygateway/gateway-runtime/python-executor/executor/translator.pygateway/gateway-runtime/python-executor/tests/__init__.pygateway/gateway-runtime/python-executor/tests/test_execution_tracker.pygateway/gateway-runtime/python-executor/tests/test_sdk_types.pygateway/gateway-runtime/python-executor/tests/test_server.pygateway/gateway-runtime/python-executor/tests/test_translator.pysdk-python/README.mdsdk-python/pyproject.tomlsdk-python/src/apip_sdk_core/__init__.pysdk-python/src/apip_sdk_core/policy/__init__.pysdk-python/src/apip_sdk_core/policy/v1alpha2/__init__.pysdk-python/src/apip_sdk_core/policy/v1alpha2/actions.pysdk-python/src/apip_sdk_core/policy/v1alpha2/policy.pysdk-python/src/apip_sdk_core/policy/v1alpha2/types.pysdk-python/src/apip_sdk_core/py.typed
💤 Files with no reviewable changes (3)
- gateway/gateway-controller/default-policies/python-streaming-logger.yaml
- gateway/gateway-controller/default-policies/python-header-modifier.yaml
- gateway/gateway-controller/default-policies/python-body-buffer-modifier.yaml
Prefer pip candidates when resolving python policies by requiring IsPipPackage, and add a test to ensure non-pip local candidates are skipped. Reject unsupported named direct pip references ("name @ url") with a clear error. Introduce buildExactIndexedPipSpec to construct exact indexed pip specs (including index URL) and avoid leaking raw index URLs in logs by sanitizing the logged value. Add unit tests for the new behaviors and update the PythonSourceDir comment to reflect extracted pip policies.
Add standalone apip-sdk-core Python package and packaging metadata for an initial 0.1.0 release. Files added: README (detailed usage, examples and API reference), CHANGELOG, LICENSE (Apache-2.0), .gitignore, tests, and package pyproject.toml updates (classifiers, license file, URLs, sdist/wheel settings, and dev extras). Update package root (__init__) to re-export versioned policy symbols and expose __version__ from package metadata. Add basic smoke tests for the public API and headers behavior. Remove the top-level py.typed marker (package-level typed marker expected in src/apip_sdk_core).
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sdk-python/tests/test_public_api.py (1)
10-14: Optional: prefer installed-package resolution oversys.pathinjection.Inserting
src/intosys.pathworks but bypasses the installed distribution, which can mask packaging regressions (e.g., missingpy.typedin the built wheel would still passtest_package_includes_typed_markerbecause it reads from the source tree). Consider running these smoke tests against the installedapip-sdk-core(e.g., viapip install -e .in CI) and dropping the path shim, so the tests exercise the same artifact users consume.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-python/tests/test_public_api.py` around lines 10 - 14, The test inserts the source tree into sys.path via ROOT and SRC which bypasses the installed distribution and can mask packaging issues; remove the sys.path insertion (the block that checks and inserts str(SRC) into sys.path) and instead ensure CI/local test setup installs the package prior to running tests (e.g., run pip install -e . or pip install . in setup steps) so tests import the installed apip-sdk-core package rather than loading from the source tree.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@sdk-python/tests/test_public_api.py`:
- Around line 10-14: The test inserts the source tree into sys.path via ROOT and
SRC which bypasses the installed distribution and can mask packaging issues;
remove the sys.path insertion (the block that checks and inserts str(SRC) into
sys.path) and instead ensure CI/local test setup installs the package prior to
running tests (e.g., run pip install -e . or pip install . in setup steps) so
tests import the installed apip-sdk-core package rather than loading from the
source tree.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 803736a9-d04e-4272-b363-abba07745092
📒 Files selected for processing (8)
sdk-python/.gitignoresdk-python/CHANGELOG.mdsdk-python/LICENSEsdk-python/README.mdsdk-python/pyproject.tomlsdk-python/src/apip_sdk_core/__init__.pysdk-python/tests/__init__.pysdk-python/tests/test_public_api.py
✅ Files skipped from review due to trivial changes (4)
- sdk-python/tests/init.py
- sdk-python/LICENSE
- sdk-python/.gitignore
- sdk-python/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- sdk-python/README.md
- sdk-python/pyproject.toml
Co-authored-by: Renuka Piyumal Fernando <renukapiyumal@gmail.com>
Point README license badge and license reference to the repository LICENSE on GitHub instead of a local file. Also remove the author's email from the authors list in pyproject.toml (metadata cleanup/privacy). No functional code changes.
renuka-fernando
approved these changes
Apr 27, 2026
Contributor
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Delete sdk-python/LICENSE and remove license references from sdk-python/pyproject.toml: remove the license = { file = "LICENSE" } setting and the "LICENSE" entry from [tool.hatch.build.targets.sdist]. Other package metadata and included files remain unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gateway/gateway-runtime/python-executor/executor/translator.py (1)
26-56:⚠️ Potential issue | 🔴 CriticalCritical: 27 of 28 imported symbols are not exported by
apip_sdk_core.The import statement references symbols that are not available at the package root. Of the 28 symbols imported from
apip_sdk_core(lines 26–56), onlyProcessingModeis currently exported. The remaining 27 symbols—includingAuthContext,Body,DropHeaderAction,ExecutionPhase,ForwardRequestChunk, and others—are missing from the root__init__.pyand will cause anImportErrorat runtime.Either these symbols must be added to
apip_sdk_core/__init__.pyfor root-level re-export, or the import must be adjusted to import from the correct submodule locations where these types are defined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-runtime/python-executor/executor/translator.py` around lines 26 - 56, The import in translator.py pulls 28 symbols from apip_sdk_core but only ProcessingMode is actually re-exported; to fix this either add the missing symbols (AuthContext, Body, DownstreamResponseHeaderModifications, DownstreamResponseModifications, DropHeaderAction, ExecutionPhase, ForwardRequestChunk, ForwardResponseChunk, Headers, ImmediateResponse, PolicyMetadata, RequestAction, RequestContext, RequestHeaderAction, RequestHeaderContext, RequestStreamContext, ResponseAction, ResponseContext, ResponseHeaderAction, ResponseHeaderContext, ResponseStreamContext, SharedContext, StreamBody, StreamingRequestAction, StreamingResponseAction, TerminateResponseChunk, UpstreamRequestHeaderModifications, UpstreamRequestModifications) into apip_sdk_core/__init__.py as re-exports, or change translator.py to import each missing symbol from its actual submodule where it is defined; update the import block in translator.py accordingly so only valid root exports or correct submodule paths are imported.
🧹 Nitpick comments (10)
gateway/gateway-builder/internal/buildfile/generator.go (1)
186-200: Consider logging when the pip-spec fallback is taken.The exact-match path (line 188) is the intended primary selector; the fallback at lines 193–200 will pick the first pip candidate whose
OriginalPipSpecdoes not matchme.PipPackage. This silently substitutes a different resolved pip spec into the manifest at line 209. Aslog.Warnhere (withme.Name,me.PipPackage, and the chosenfound.OriginalPipSpec) would aid debugging when discovery and the build entry drift apart.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-builder/internal/buildfile/generator.go` around lines 186 - 200, Add a warning log when the pip-spec fallback path is used in the PipPackage resolution in generator.go: after the fallback loop that sets found from candidates (the block using variables me, candidates, found), call slog.Warn including me.Name, the requested me.PipPackage and the chosen found.OriginalPipSpec to indicate a substitution; ensure this is emitted only when found != nil and found.OriginalPipSpec != me.PipPackage so maintainers can trace when the resolved pip spec differs from the requested one.gateway/gateway-runtime/Dockerfile (1)
142-143: Optional: pinpip,setuptools, andwheelfor reproducible builds.Upgrading without version pins makes the
python-depsstage non-deterministic across rebuilds and may surface upstream regressions unexpectedly. Consider pinning known-good versions (and refreshing them deliberately).Suggested pinning
-# Upgrade pip and setuptools so PEP 621 (pyproject.toml) and recent git dependencies are supported -RUN pip3 install --no-cache-dir --upgrade pip setuptools wheel +# Upgrade pip, setuptools, and wheel (pinned for reproducible builds) so PEP 621 (pyproject.toml) +# and recent git dependencies are supported. +RUN pip3 install --no-cache-dir --upgrade \ + pip==<pinned> \ + setuptools==<pinned> \ + wheel==<pinned>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-runtime/Dockerfile` around lines 142 - 143, The Dockerfile currently upgrades pip/setuptools/wheel non-deterministically via the RUN pip3 install --no-cache-dir --upgrade pip setuptools wheel step; replace that with pinned versions (explicit version strings for pip, setuptools, wheel) in that same RUN instruction and document/update them deliberately so the python-deps stage is reproducible and only changes when you intentionally bump those versions (refer to the existing RUN pip3 install ... line to locate the change).sdk-python/CHANGELOG.md (2)
10-10: Optional: add a comparison link target for[0.1.0].Keep a Changelog conventionally defines a reference link at the bottom of the file (e.g.,
[0.1.0]: https://.../releases/tag/v0.1.0) so the version header renders as a link. Not required, but improves navigation for consumers browsing on GitHub.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-python/CHANGELOG.md` at line 10, Add a reference link target for the changelog version header "[0.1.0]" so the heading becomes a clickable link; append a link definition at the bottom of CHANGELOG.md like `[0.1.0]: https://github.com/<owner>/<repo>/releases/tag/v0.1.0` (replace owner/repo as appropriate) so the existing "## [0.1.0] — 2026-04-23" header resolves to that release comparison link.
36-36: Nit: spelling consistency — "licence" vs "License".The file uses "licence" (British) while the SDK source headers and
LICENSEfile use "License" (US). Aligning with the project's existing usage keeps the changelog consistent with surrounding artifacts.Suggested change
-- Apache 2.0 licence. +- Apache 2.0 license.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-python/CHANGELOG.md` at line 36, Replace the British spelling in the changelog entry "Apache 2.0 licence." with the project's US spelling "Apache 2.0 License." — locate the exact string "Apache 2.0 licence." in CHANGELOG.md and update it to "Apache 2.0 License." to match the LICENSE header and other source file headers.sdk-python/README.md (1)
132-144: Minor: unused import in streaming example.
UpstreamRequestModificationsis imported in this example but never used in theStreamingLoggerclass body. Trim it from the import list to keep the snippet tight and copy-pastable.📝 Proposed cleanup
from apip_sdk_core import ( BodyProcessingMode, ExecutionContext, ForwardResponseChunk, ProcessingMode, ResponseContext, ResponseAction, ResponseStreamContext, StreamBody, StreamingResponseAction, StreamingResponsePolicy, - UpstreamRequestModifications, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-python/README.md` around lines 132 - 144, The import list for the streaming example includes an unused symbol; remove UpstreamRequestModifications from the imports shown and update the example so only used symbols (e.g., StreamingLogger, StreamingResponseAction, ResponseStreamContext, StreamBody, etc.) are imported; locate the import block that currently lists UpstreamRequestModifications and delete that identifier so the StreamingLogger example no longer shows an unused import.gateway/gateway-builder/internal/discovery/discovery_test.go (1)
942-1008: Integration test depends on environment-specific tooling.
TestFetchPipPackage_GitFileReferencerequiresgit,pip, and the ability to build a wheel viasetuptools/wheelfrompyproject.toml. The test correctly skips whengitorpipis unavailable, but a missingsetuptools/wheel(or PEP 517 build backend resolution issues, e.g., no network for--no-build-isolationnot being set) could still cause flakes in constrained CI environments. Consider also handling the build-backend dependency case (e.g., adding--no-build-isolationor skipping when wheel building fails for environment reasons). Otherwise the test logic itself is sound.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-builder/internal/discovery/discovery_test.go` around lines 942 - 1008, TestFetchPipPackage_GitFileReference can still flake when wheel/setuptools build backend isn’t available; modify the test to detect or avoid build-time network/isolation issues by either (a) attempting a quick build check before running the main test and skipping if the PEP517 backend can't be resolved, or (b) invoking pip via resolvePipExecutable and using a wheel install option that avoids build isolation (e.g. add --no-build-isolation and/or --no-deps when calling pip in FetchPipPackage) and fall back to skipping the test on build failures. Locate the test TestFetchPipPackage_GitFileReference and the code path that calls FetchPipPackage (and resolvePipExecutable) and implement: pre-check for wheel/setuptools availability or update the pip invocation in FetchPipPackage to include --no-build-isolation/--no-deps and treat build errors as a test skip instead of a hard failure.gateway/gateway-builder/internal/discovery/python_module.go (2)
562-671: Consider operational guidance forgit ls-remotetimeout.The 30s context timeout for
git ls-remote --tagsis reasonable for typical hosts, but for large repos or slow links it may flake. Consider making this configurable via env/option, or document the constraint. Also, ranking by (minor, patch) descending is correct for the documented ref shapes; any non-numeric suffix (e.g.,-rc1) is silently skipped, which matches the intent of selecting only released SemVer patches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-builder/internal/discovery/python_module.go` around lines 562 - 671, The hardcoded 30s timeout in resolveVCSVersion (context.WithTimeout(ctx, 30*time.Second)) can flake on large/slow repos; make the timeout configurable (e.g., read an env var like GIT_LS_REMOTE_TIMEOUT or accept a timeout option on the caller) and use that parsed duration as the context timeout with a sensible default of 30s, update the function or its caller to pass the configurable duration, and ensure any validation/parsing errors fall back to the default and are logged so the behavior is clear.
39-45: Consider tighter anchoring on the VCS ref regex patterns.
majorOnlyRefPattern(v(\d+)$) andminorOnlyRefPattern(v(\d+)\.(\d+)$) are not anchored at the start, so refs likerelease-v1would be classified as major-only. The test coverage confirms only standard refs following thepolicies/<name>/v<major>[.<minor>]convention are used in practice, making this a robustness concern rather than a defect. To harden classification, consider anchoring the leading boundary:(^|/)v(\d+)$and(^|/)v(\d+)\.(\d+)$.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-builder/internal/discovery/python_module.go` around lines 39 - 45, The VCS ref regexes majorOnlyRefPattern and minorOnlyRefPattern are only anchored at the end and mis-classify refs like "release-v1"; update their patterns to require the ref start or a path separator before the version (e.g. use a leading boundary like '(^|/)v(\\d+)$' and '(^|/)v(\\d+)\\.(\\d+)$') by replacing the current regexp.MustCompile calls for majorOnlyRefPattern and minorOnlyRefPattern with these tighter patterns so they only match standard refs or path-suffixed refs.sdk-python/pyproject.toml (2)
47-55: Optional: reconsider shippingtestsin the sdist.Including
testsin the sdist is a valid choice (useful for downstream packagers running the test suite), but if the intent is a leaner distribution for end users, consider dropping it fromonly-includeand keeping tests source-only. No change required if shipping tests is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-python/pyproject.toml` around lines 47 - 55, The sdist configuration under [tool.hatch.build.targets.sdist] currently includes "tests" in the only-include list, which causes test sources to be packaged into the source distribution; if you want a leaner sdist for end users remove "tests" from the only-include array (edit the only-include setting in pyproject.toml) otherwise leave it as-is if including tests is intentional.
25-25: Switch to PEP 639 SPDX license expression for modernization.The
License :: OSI Approved :: Apache Software Licenseclassifier is deprecated in favor of SPDX expressions. Sincelicense = { file = "LICENSE" }is already declared, you can optionally modernize tolicense = "Apache-2.0"and remove the deprecated classifier. Hatchling has supported PEP 639 since v1.5.0 (2022), so version constraints are not a concern for current deployments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-python/pyproject.toml` at line 25, Update the deprecated Trove classifier string "License :: OSI Approved :: Apache Software License" to use a PEP 639 SPDX expression by setting license = "Apache-2.0" (or add that alongside the existing license = { file = "LICENSE" }), and remove the deprecated classifier line from the classifiers list; ensure the project still retains or references the LICENSE file via the existing license = { file = "LICENSE" } entry if you prefer keeping the file pointer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk-python/src/apip_sdk_core/__init__.py`:
- Line 22: The current __all__ assignment mixes an unpacked attribute which
linters flag; replace the single-line unpack with an explicit string-only list
construction: create a local list (e.g., all_names = list(policy.__all__)) then
extend or concatenate with the literal strings "policy" and "__version__", and
finally assign that list to __all__; this ensures __all__ contains only strings
and resolves PLE0604 while keeping references to policy and __version__ intact.
---
Outside diff comments:
In `@gateway/gateway-runtime/python-executor/executor/translator.py`:
- Around line 26-56: The import in translator.py pulls 28 symbols from
apip_sdk_core but only ProcessingMode is actually re-exported; to fix this
either add the missing symbols (AuthContext, Body,
DownstreamResponseHeaderModifications, DownstreamResponseModifications,
DropHeaderAction, ExecutionPhase, ForwardRequestChunk, ForwardResponseChunk,
Headers, ImmediateResponse, PolicyMetadata, RequestAction, RequestContext,
RequestHeaderAction, RequestHeaderContext, RequestStreamContext, ResponseAction,
ResponseContext, ResponseHeaderAction, ResponseHeaderContext,
ResponseStreamContext, SharedContext, StreamBody, StreamingRequestAction,
StreamingResponseAction, TerminateResponseChunk,
UpstreamRequestHeaderModifications, UpstreamRequestModifications) into
apip_sdk_core/__init__.py as re-exports, or change translator.py to import each
missing symbol from its actual submodule where it is defined; update the import
block in translator.py accordingly so only valid root exports or correct
submodule paths are imported.
---
Nitpick comments:
In `@gateway/gateway-builder/internal/buildfile/generator.go`:
- Around line 186-200: Add a warning log when the pip-spec fallback path is used
in the PipPackage resolution in generator.go: after the fallback loop that sets
found from candidates (the block using variables me, candidates, found), call
slog.Warn including me.Name, the requested me.PipPackage and the chosen
found.OriginalPipSpec to indicate a substitution; ensure this is emitted only
when found != nil and found.OriginalPipSpec != me.PipPackage so maintainers can
trace when the resolved pip spec differs from the requested one.
In `@gateway/gateway-builder/internal/discovery/discovery_test.go`:
- Around line 942-1008: TestFetchPipPackage_GitFileReference can still flake
when wheel/setuptools build backend isn’t available; modify the test to detect
or avoid build-time network/isolation issues by either (a) attempting a quick
build check before running the main test and skipping if the PEP517 backend
can't be resolved, or (b) invoking pip via resolvePipExecutable and using a
wheel install option that avoids build isolation (e.g. add --no-build-isolation
and/or --no-deps when calling pip in FetchPipPackage) and fall back to skipping
the test on build failures. Locate the test TestFetchPipPackage_GitFileReference
and the code path that calls FetchPipPackage (and resolvePipExecutable) and
implement: pre-check for wheel/setuptools availability or update the pip
invocation in FetchPipPackage to include --no-build-isolation/--no-deps and
treat build errors as a test skip instead of a hard failure.
In `@gateway/gateway-builder/internal/discovery/python_module.go`:
- Around line 562-671: The hardcoded 30s timeout in resolveVCSVersion
(context.WithTimeout(ctx, 30*time.Second)) can flake on large/slow repos; make
the timeout configurable (e.g., read an env var like GIT_LS_REMOTE_TIMEOUT or
accept a timeout option on the caller) and use that parsed duration as the
context timeout with a sensible default of 30s, update the function or its
caller to pass the configurable duration, and ensure any validation/parsing
errors fall back to the default and are logged so the behavior is clear.
- Around line 39-45: The VCS ref regexes majorOnlyRefPattern and
minorOnlyRefPattern are only anchored at the end and mis-classify refs like
"release-v1"; update their patterns to require the ref start or a path separator
before the version (e.g. use a leading boundary like '(^|/)v(\\d+)$' and
'(^|/)v(\\d+)\\.(\\d+)$') by replacing the current regexp.MustCompile calls for
majorOnlyRefPattern and minorOnlyRefPattern with these tighter patterns so they
only match standard refs or path-suffixed refs.
In `@gateway/gateway-runtime/Dockerfile`:
- Around line 142-143: The Dockerfile currently upgrades pip/setuptools/wheel
non-deterministically via the RUN pip3 install --no-cache-dir --upgrade pip
setuptools wheel step; replace that with pinned versions (explicit version
strings for pip, setuptools, wheel) in that same RUN instruction and
document/update them deliberately so the python-deps stage is reproducible and
only changes when you intentionally bump those versions (refer to the existing
RUN pip3 install ... line to locate the change).
In `@sdk-python/CHANGELOG.md`:
- Line 10: Add a reference link target for the changelog version header
"[0.1.0]" so the heading becomes a clickable link; append a link definition at
the bottom of CHANGELOG.md like `[0.1.0]:
https://github.com/<owner>/<repo>/releases/tag/v0.1.0` (replace owner/repo as
appropriate) so the existing "## [0.1.0] — 2026-04-23" header resolves to that
release comparison link.
- Line 36: Replace the British spelling in the changelog entry "Apache 2.0
licence." with the project's US spelling "Apache 2.0 License." — locate the
exact string "Apache 2.0 licence." in CHANGELOG.md and update it to "Apache 2.0
License." to match the LICENSE header and other source file headers.
In `@sdk-python/pyproject.toml`:
- Around line 47-55: The sdist configuration under
[tool.hatch.build.targets.sdist] currently includes "tests" in the only-include
list, which causes test sources to be packaged into the source distribution; if
you want a leaner sdist for end users remove "tests" from the only-include array
(edit the only-include setting in pyproject.toml) otherwise leave it as-is if
including tests is intentional.
- Line 25: Update the deprecated Trove classifier string "License :: OSI
Approved :: Apache Software License" to use a PEP 639 SPDX expression by setting
license = "Apache-2.0" (or add that alongside the existing license = { file =
"LICENSE" }), and remove the deprecated classifier line from the classifiers
list; ensure the project still retains or references the LICENSE file via the
existing license = { file = "LICENSE" } entry if you prefer keeping the file
pointer.
In `@sdk-python/README.md`:
- Around line 132-144: The import list for the streaming example includes an
unused symbol; remove UpstreamRequestModifications from the imports shown and
update the example so only used symbols (e.g., StreamingLogger,
StreamingResponseAction, ResponseStreamContext, StreamBody, etc.) are imported;
locate the import block that currently lists UpstreamRequestModifications and
delete that identifier so the StreamingLogger example no longer shows an unused
import.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f72524d8-c019-449e-a32f-d7a91420269b
📒 Files selected for processing (40)
gateway/gateway-builder/internal/buildfile/generator.gogateway/gateway-builder/internal/buildfile/generator_test.gogateway/gateway-builder/internal/discovery/discovery_test.gogateway/gateway-builder/internal/discovery/manifest.gogateway/gateway-builder/internal/discovery/python_module.gogateway/gateway-builder/internal/discovery/python_module_test.gogateway/gateway-builder/internal/policyengine/generator.gogateway/gateway-builder/internal/policyengine/policyengine_test.gogateway/gateway-builder/pkg/types/policy.gogateway/gateway-controller/default-policies/python-body-buffer-modifier.yamlgateway/gateway-controller/default-policies/python-header-modifier.yamlgateway/gateway-controller/default-policies/python-streaming-logger.yamlgateway/gateway-runtime/Dockerfilegateway/gateway-runtime/python-executor/executor/__init__.pygateway/gateway-runtime/python-executor/executor/execution_tracker.pygateway/gateway-runtime/python-executor/executor/instance_store.pygateway/gateway-runtime/python-executor/executor/policy_loader.pygateway/gateway-runtime/python-executor/executor/server.pygateway/gateway-runtime/python-executor/executor/translator.pygateway/gateway-runtime/python-executor/tests/__init__.pygateway/gateway-runtime/python-executor/tests/test_execution_tracker.pygateway/gateway-runtime/python-executor/tests/test_sdk_types.pygateway/gateway-runtime/python-executor/tests/test_server.pygateway/gateway-runtime/python-executor/tests/test_translator.pysdk-python/.gitignoresdk-python/CHANGELOG.mdsdk-python/LICENSEsdk-python/README.mdsdk-python/pyproject.tomlsdk-python/src/apip_sdk_core/__init__.pysdk-python/src/apip_sdk_core/policy/__init__.pysdk-python/src/apip_sdk_core/policy/v1alpha2/__init__.pysdk-python/src/apip_sdk_core/policy/v1alpha2/actions.pysdk-python/src/apip_sdk_core/policy/v1alpha2/policy.pysdk-python/src/apip_sdk_core/policy/v1alpha2/types.pysdk-python/src/apip_sdk_core/py.typedsdk-python/src/wso2_gateway_policy_sdk/__init__.pysdk-python/src/wso2_gateway_policy_sdk/py.typedsdk-python/tests/__init__.pysdk-python/tests/test_public_api.py
💤 Files with no reviewable changes (4)
- gateway/gateway-controller/default-policies/python-streaming-logger.yaml
- gateway/gateway-controller/default-policies/python-header-modifier.yaml
- gateway/gateway-controller/default-policies/python-body-buffer-modifier.yaml
- sdk-python/src/wso2_gateway_policy_sdk/init.py
Add a new gateway policy manifest (gateway/gateway-controller/default-policies/prompt-compressor.yaml) that compresses prompt text before upstream LLM calls. The policy targets a single string via jsonPath (default $.messages[0].content), supports ordered compression rules (upperTokenLimit, type: ratio|token, value) including a -1 fallback, and supports selective compression tags. Adjust sdk-python/src/apip_sdk_core/__init__.py to build __all__ by converting policy.__all__ to a list and extending it with "policy" and "__version__" to ensure consistent exports.
Contributor
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
gateway/gateway-controller/default-policies/prompt-compressor.yaml (1)
30-56: Consider tightening numeric constraints on rule fields.The schema currently allows any integer for
upperTokenLimitand any number forvalue, but only-1is documented as a valid sentinel and negativevaluehas no defined semantic. Adding bounds would let JSON Schema validation catch malformed rules at policy load/apply time rather than at runtime.♻️ Suggested constraints
upperTokenLimit: type: integer + minimum: -1 description: | Inclusive upper bound for the estimated token count. Use -1 as the fallback rule for all remaining cases. @@ value: type: number + minimum: 0 + exclusiveMinimum: true description: | Rule value. For "ratio", use a retained-size ratio such as 0.8. For "token", use the target retained token estimate.Note: for
ratio, an additionalmaximum: 1could be enforced if values >1 are not meaningful — confirm against the runtime behavior before tightening further.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-controller/default-policies/prompt-compressor.yaml` around lines 30 - 56, The schema allows unbounded integers for upperTokenLimit and any number for value; tighten these by adding numeric constraints: set upperTokenLimit to an integer with minimum -1 (and a sensible maximum if desired), and constrain value conditionally based on type — for type:"ratio" require a number with minimum 0 (and optionally maximum 1 if runtime forbids >1), and for type:"token" require an integer with minimum 0; implement these using JSON Schema keywords (minimum/maximum/type and an if/then on the parent items schema keyed by type) so invalid rules are rejected at load time.sdk-python/src/apip_sdk_core/__init__.py (1)
22-23: Optional: sort__all__to satisfy Ruff RUF022.Static analysis flags
__all__as unsorted. Sorting is purely stylistic and behavior-preserving.Proposed adjustment
-__all__ = list(policy.__all__) -__all__.extend(["policy", "__version__"]) +__all__ = sorted([*policy.__all__, "policy", "__version__"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-python/src/apip_sdk_core/__init__.py` around lines 22 - 23, The __all__ list defined and extended here (__all__ = list(policy.__all__); __all__.extend(["policy", "__version__"])) is flagged as unsorted by Ruff RUF022; to fix, sort the names after building the list (e.g., call sorted(...) or __all__.sort()) so the exported symbol list is alphabetically ordered while preserving the same contents, making the change in the __all__ construction in __init__.py (the symbols referenced are __all__, policy, and __version__).sdk-python/tests/test_public_api.py (1)
32-39: Optional: also assert defensive copy onget_all().The companion test in
gateway/gateway-runtime/python-executor/tests/test_sdk_types.pymutates bothheaders.get(...)andheaders.get_all()[...]and asserts the underlying state is unaffected. This smoke test only covers theget()path, leaving theget_all()defensive-copy contract untested at the SDK package boundary.♻️ Proposed addition
values = headers.get("x-test") values.append("three") + all_headers = headers.get_all() + all_headers["x-test"].append("four") + self.assertEqual(headers.get("X-Test"), ["one", "two"]) self.assertEqual(headers.get_all(), {"x-test": ["one", "two"]})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-python/tests/test_public_api.py` around lines 32 - 39, Update the test_headers_are_case_insensitive_and_defensive test to also verify that Headers.get_all() returns a defensive copy: call headers.get_all(), mutate the returned mapping or its list values (e.g., append or reassign an entry) and then assert that subsequent calls to headers.get("X-Test") and headers.get_all() still reflect the original data; reference the Headers class and its get_all and get methods when locating where to add this assertion.sdk-python/README.md (1)
132-144: Optional: drop unused import in the StreamingLogger example.
UpstreamRequestModificationsis imported but never referenced insideStreamingLogger. Removing it keeps the example minimal and avoids implying it's part of the streaming-response flow.📝 Proposed doc tweak
from apip_sdk_core import ( BodyProcessingMode, ExecutionContext, ForwardResponseChunk, ProcessingMode, ResponseContext, ResponseAction, ResponseStreamContext, StreamBody, StreamingResponseAction, StreamingResponsePolicy, - UpstreamRequestModifications, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-python/README.md` around lines 132 - 144, The README streaming example imports UpstreamRequestModifications but never uses it in the StreamingLogger example; remove UpstreamRequestModifications from the import list to keep the example minimal and avoid implying it's part of the streaming-response flow—update the import tuple that currently includes BodyProcessingMode, ExecutionContext, ForwardResponseChunk, ProcessingMode, ResponseContext, ResponseAction, ResponseStreamContext, StreamBody, StreamingResponseAction, StreamingResponsePolicy, UpstreamRequestModifications to omit UpstreamRequestModifications while leaving the rest unchanged.sdk-python/pyproject.toml (1)
10-23: Consider adding an upper bound torequires-python.Classifiers enumerate Python 3.10–3.13 and include
Programming Language :: Python :: 3 :: Only, butrequires-python = ">=3.10"has no upper bound. If 3.13 is the intended ceiling, tighten the constraint (e.g.,">=3.10,<3.14") so installers reject unsupported interpreters; otherwise, drop the implication of a bounded range from the classifiers.Proposed change
-requires-python = ">=3.10" +requires-python = ">=3.10,<3.14"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-python/pyproject.toml` around lines 10 - 23, The requires-python entry currently allows any Python >=3.10 while the classifiers list Python 3.10–3.13 (and "Python :: 3 :: Only"); update the package metadata to make them consistent: either add an upper bound to requires-python (e.g., change requires-python to reflect the intended ceiling like ">=3.10,<3.14") or remove the bounded-version classifiers; modify the requires-python field (symbol: requires-python) and/or the classifiers array (symbol: classifiers) so installers and classifiers agree on supported Python versions.gateway/gateway-builder/internal/discovery/python_module.go (3)
562-585: Minor: detect missinggitupfront for a clearer error.
resolveVCSVersionand the wider VCS pip flow shell out togitwithout first probing the PATH (unlikeresolvePipExecutablefor pip). When git is absent, callers get a genericexec: "git": executable file not founderror wrapped under "git ls-remote failed for …", which is actionable but not as precise as the pip-availability path. Consider an explicitexec.LookPath("git")check at the top offetchVCSPipPackage(orresolveVCSVersion) to surface a clearer "git executable not found; required for VCS pip packages" message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-builder/internal/discovery/python_module.go` around lines 562 - 585, Add an upfront check for the git executable to produce a clearer error: in resolveVCSVersion (or fetchVCSPipPackage) call exec.LookPath("git") before running exec.CommandContext and if it returns an error return a descriptive error like "git executable not found; required for VCS pip packages" (include sanitized repo URL or context where appropriate); keep the existing behavior for all other git command failures so only the missing-git case is handled specially.
165-176: Optional: drop the unreachable default and add an explicitvcsVersionNonecase.The
defaultbranch inString()is currently used to handlevcsVersionNone. An explicitcase vcsVersionNone:makes intent clearer and reservesdefaultfor any future enum values that should be flagged (e.g.,return "unknown").Suggested adjustment
switch t { case vcsVersionExact: return "exact" case vcsVersionMinorOnly: return "minor-only" case vcsVersionMajorOnly: return "major-only" + case vcsVersionNone: + return "none" default: - return "none" + return "unknown" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-builder/internal/discovery/python_module.go` around lines 165 - 176, In the vcsVersionType.String() method, replace the current default branch with an explicit case for vcsVersionNone that returns "none" and add a fallback default that returns an "unknown" (or similar) string to catch any future enum values; update the switch in String() (function name: String, type: vcsVersionType, constants: vcsVersionExact, vcsVersionMinorOnly, vcsVersionMajorOnly, vcsVersionNone) so intent is explicit and unexpected values are flagged by the default.
327-336: Document the build-backend dependency for--no-build-isolation.
pip wheel --no-build-isolationskips PEP 517 build isolation, sosetuptools/wheel(and any other build requirements declared by the source distribution) must be importable from the active Python. CI Dockerfiles upgrade pip/setuptools/wheel for this reason, but local builds against this code path will fail with a build-backend error if those packages are missing. A brief code comment at lines 327-334 explaining the system-package expectation would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-builder/internal/discovery/python_module.go` around lines 327 - 336, Add a brief code comment above the block that creates wheelDir and calls runPipCommand with "wheel" and "--no-build-isolation" explaining that using --no-build-isolation requires the build-backend dependencies (e.g., setuptools, wheel and any PEP 517 build-requirements) to be importable in the active Python environment; reference the resolvedSpec invocation so maintainers know local builds may fail unless pip/setuptools/wheel are installed or CI Dockerfiles upgrade them. Locate the comment near the wheelDir creation and the runPipCommand(...) call so it directly documents the expectation for build-backend packages when building the resolvedSpec.gateway/gateway-builder/internal/discovery/python_module_test.go (1)
64-80: Note:~=1.2is rejected even though it is valid per PEP 440.
ParsePipPackageRefonly accepts~=<major>.0and~=<major>.<minor>.0(versions ending in.0), soprompt-compressor~=1.2is treated as invalid by design. The test confirms current behavior, but if users pass an otherwise pip-valid spec like~=1.2, the error message ("invalid pip spec…") may be confusing. Consider clarifying the error or expanding the documentation inParsePipPackageRefto make this restriction explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-builder/internal/discovery/python_module_test.go` around lines 64 - 80, The test shows ParsePipPackageRef rejects valid PEP 440 specs like "~=1.2" because the parser currently only accepts ~= with trailing .0 segments; update ParsePipPackageRef to either accept the broader PEP 440 "~=X.Y" form or make the rejection clearer by changing its error message and docs: modify the error returned from ParsePipPackageRef (and any related validation helper) to include the specific restriction (e.g., "unsupported ~= spec: only major or major.minor.0 accepted") and add a short note in the function comment/docs explaining the intended limitation so users aren’t confused; adjust the TestParsePipPackageRef_Invalid expectations or add a new test case that asserts the more explicit error text.gateway/gateway-builder/internal/buildfile/generator_test.go (1)
462-492: Optional: also cover the explicitOriginalPipSpecmatch path here.This test only adds a single candidate, so resolution is satisfied by the
len(candidates) == 1short-circuit atgenerator.golines 141-142 rather than the new pip-specific selection loop. SettingOriginalPipSpec: "prompt-compressor~=0.0"on the candidate would additionally exercise the exact-match branch (line 188) for this single-candidate scenario.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/gateway-builder/internal/buildfile/generator_test.go` around lines 462 - 492, The test TestWriteBuildManifestWithVersions_PipPackageUsesResolvedSpec only exercised the single-candidate short-circuit; modify the discovered candidate used in the test to include OriginalPipSpec: "prompt-compressor~=0.0" so the code path that checks for an exact OriginalPipSpec match inside WriteBuildManifestWithVersions (the pip-specific selection logic) is exercised; keep the rest of the test assertions the same to verify the resolved PipSpec is written.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gateway/gateway-builder/internal/discovery/discovery_test.go`:
- Around line 942-1008: The test TestFetchPipPackage_GitFileReference relies on
system-installed build backends (setuptools/wheel) because
fetchVCSPipPackage/FetchPipPackage runs pip wheel --no-build-isolation; add a
skip guard alongside the existing git and pip checks by probing for setuptools
and wheel (e.g., call the resolved pip/python to run a short import check or pip
show for "setuptools" and "wheel") using the same pattern as
resolvePipExecutable and skip the test if either package is missing; insert this
check at the top of TestFetchPipPackage_GitFileReference near the git/pip guards
so the test fails fast and skips gracefully when the build backends are not
available.
In `@gateway/gateway-controller/default-policies/prompt-compressor.yaml`:
- Around line 22-29: The schema for the "rules" array in prompt-compressor.yaml
doesn't enforce presence of a catch-all rule (upperTokenLimit: -1); either
update the schema to require at least one rule with upperTokenLimit equal to -1
(use a JSON Schema "contains" constraint targeting items where "upperTokenLimit"
is -1 and set "minContains": 1 or equivalent), or add a runtime validation in
the policy apply path that inspects the "rules" array and throws a clear error
when no item has upperTokenLimit === -1; reference the "rules" array in
prompt-compressor.yaml and the "upperTokenLimit" field when implementing the
change.
---
Nitpick comments:
In `@gateway/gateway-builder/internal/buildfile/generator_test.go`:
- Around line 462-492: The test
TestWriteBuildManifestWithVersions_PipPackageUsesResolvedSpec only exercised the
single-candidate short-circuit; modify the discovered candidate used in the test
to include OriginalPipSpec: "prompt-compressor~=0.0" so the code path that
checks for an exact OriginalPipSpec match inside WriteBuildManifestWithVersions
(the pip-specific selection logic) is exercised; keep the rest of the test
assertions the same to verify the resolved PipSpec is written.
In `@gateway/gateway-builder/internal/discovery/python_module_test.go`:
- Around line 64-80: The test shows ParsePipPackageRef rejects valid PEP 440
specs like "~=1.2" because the parser currently only accepts ~= with trailing .0
segments; update ParsePipPackageRef to either accept the broader PEP 440 "~=X.Y"
form or make the rejection clearer by changing its error message and docs:
modify the error returned from ParsePipPackageRef (and any related validation
helper) to include the specific restriction (e.g., "unsupported ~= spec: only
major or major.minor.0 accepted") and add a short note in the function
comment/docs explaining the intended limitation so users aren’t confused; adjust
the TestParsePipPackageRef_Invalid expectations or add a new test case that
asserts the more explicit error text.
In `@gateway/gateway-builder/internal/discovery/python_module.go`:
- Around line 562-585: Add an upfront check for the git executable to produce a
clearer error: in resolveVCSVersion (or fetchVCSPipPackage) call
exec.LookPath("git") before running exec.CommandContext and if it returns an
error return a descriptive error like "git executable not found; required for
VCS pip packages" (include sanitized repo URL or context where appropriate);
keep the existing behavior for all other git command failures so only the
missing-git case is handled specially.
- Around line 165-176: In the vcsVersionType.String() method, replace the
current default branch with an explicit case for vcsVersionNone that returns
"none" and add a fallback default that returns an "unknown" (or similar) string
to catch any future enum values; update the switch in String() (function name:
String, type: vcsVersionType, constants: vcsVersionExact, vcsVersionMinorOnly,
vcsVersionMajorOnly, vcsVersionNone) so intent is explicit and unexpected values
are flagged by the default.
- Around line 327-336: Add a brief code comment above the block that creates
wheelDir and calls runPipCommand with "wheel" and "--no-build-isolation"
explaining that using --no-build-isolation requires the build-backend
dependencies (e.g., setuptools, wheel and any PEP 517 build-requirements) to be
importable in the active Python environment; reference the resolvedSpec
invocation so maintainers know local builds may fail unless pip/setuptools/wheel
are installed or CI Dockerfiles upgrade them. Locate the comment near the
wheelDir creation and the runPipCommand(...) call so it directly documents the
expectation for build-backend packages when building the resolvedSpec.
In `@gateway/gateway-controller/default-policies/prompt-compressor.yaml`:
- Around line 30-56: The schema allows unbounded integers for upperTokenLimit
and any number for value; tighten these by adding numeric constraints: set
upperTokenLimit to an integer with minimum -1 (and a sensible maximum if
desired), and constrain value conditionally based on type — for type:"ratio"
require a number with minimum 0 (and optionally maximum 1 if runtime forbids
>1), and for type:"token" require an integer with minimum 0; implement these
using JSON Schema keywords (minimum/maximum/type and an if/then on the parent
items schema keyed by type) so invalid rules are rejected at load time.
In `@sdk-python/pyproject.toml`:
- Around line 10-23: The requires-python entry currently allows any Python
>=3.10 while the classifiers list Python 3.10–3.13 (and "Python :: 3 :: Only");
update the package metadata to make them consistent: either add an upper bound
to requires-python (e.g., change requires-python to reflect the intended ceiling
like ">=3.10,<3.14") or remove the bounded-version classifiers; modify the
requires-python field (symbol: requires-python) and/or the classifiers array
(symbol: classifiers) so installers and classifiers agree on supported Python
versions.
In `@sdk-python/README.md`:
- Around line 132-144: The README streaming example imports
UpstreamRequestModifications but never uses it in the StreamingLogger example;
remove UpstreamRequestModifications from the import list to keep the example
minimal and avoid implying it's part of the streaming-response flow—update the
import tuple that currently includes BodyProcessingMode, ExecutionContext,
ForwardResponseChunk, ProcessingMode, ResponseContext, ResponseAction,
ResponseStreamContext, StreamBody, StreamingResponseAction,
StreamingResponsePolicy, UpstreamRequestModifications to omit
UpstreamRequestModifications while leaving the rest unchanged.
In `@sdk-python/src/apip_sdk_core/__init__.py`:
- Around line 22-23: The __all__ list defined and extended here (__all__ =
list(policy.__all__); __all__.extend(["policy", "__version__"])) is flagged as
unsorted by Ruff RUF022; to fix, sort the names after building the list (e.g.,
call sorted(...) or __all__.sort()) so the exported symbol list is
alphabetically ordered while preserving the same contents, making the change in
the __all__ construction in __init__.py (the symbols referenced are __all__,
policy, and __version__).
In `@sdk-python/tests/test_public_api.py`:
- Around line 32-39: Update the test_headers_are_case_insensitive_and_defensive
test to also verify that Headers.get_all() returns a defensive copy: call
headers.get_all(), mutate the returned mapping or its list values (e.g., append
or reassign an entry) and then assert that subsequent calls to
headers.get("X-Test") and headers.get_all() still reflect the original data;
reference the Headers class and its get_all and get methods when locating where
to add this assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f31d73e-50b7-4b7e-84e2-364a9ef96d35
📒 Files selected for processing (40)
gateway/gateway-builder/internal/buildfile/generator.gogateway/gateway-builder/internal/buildfile/generator_test.gogateway/gateway-builder/internal/discovery/discovery_test.gogateway/gateway-builder/internal/discovery/manifest.gogateway/gateway-builder/internal/discovery/python_module.gogateway/gateway-builder/internal/discovery/python_module_test.gogateway/gateway-builder/internal/policyengine/generator.gogateway/gateway-builder/internal/policyengine/policyengine_test.gogateway/gateway-builder/pkg/types/policy.gogateway/gateway-controller/default-policies/prompt-compressor.yamlgateway/gateway-controller/default-policies/python-body-buffer-modifier.yamlgateway/gateway-controller/default-policies/python-header-modifier.yamlgateway/gateway-controller/default-policies/python-streaming-logger.yamlgateway/gateway-runtime/Dockerfilegateway/gateway-runtime/python-executor/executor/__init__.pygateway/gateway-runtime/python-executor/executor/execution_tracker.pygateway/gateway-runtime/python-executor/executor/instance_store.pygateway/gateway-runtime/python-executor/executor/policy_loader.pygateway/gateway-runtime/python-executor/executor/server.pygateway/gateway-runtime/python-executor/executor/translator.pygateway/gateway-runtime/python-executor/tests/__init__.pygateway/gateway-runtime/python-executor/tests/test_execution_tracker.pygateway/gateway-runtime/python-executor/tests/test_sdk_types.pygateway/gateway-runtime/python-executor/tests/test_server.pygateway/gateway-runtime/python-executor/tests/test_translator.pysdk-python/.gitignoresdk-python/CHANGELOG.mdsdk-python/README.mdsdk-python/pyproject.tomlsdk-python/src/apip_sdk_core/__init__.pysdk-python/src/apip_sdk_core/policy/__init__.pysdk-python/src/apip_sdk_core/policy/v1alpha2/__init__.pysdk-python/src/apip_sdk_core/policy/v1alpha2/actions.pysdk-python/src/apip_sdk_core/policy/v1alpha2/policy.pysdk-python/src/apip_sdk_core/policy/v1alpha2/types.pysdk-python/src/apip_sdk_core/py.typedsdk-python/src/wso2_gateway_policy_sdk/__init__.pysdk-python/src/wso2_gateway_policy_sdk/py.typedsdk-python/tests/__init__.pysdk-python/tests/test_public_api.py
💤 Files with no reviewable changes (4)
- gateway/gateway-controller/default-policies/python-streaming-logger.yaml
- sdk-python/src/wso2_gateway_policy_sdk/init.py
- gateway/gateway-controller/default-policies/python-header-modifier.yaml
- gateway/gateway-controller/default-policies/python-body-buffer-modifier.yaml
Update discovery tests to use resolvePipExecutable's args and skip early if setuptools or wheel are not available (run `pip show setuptools` and `pip show wheel`). This ensures the test environment can build Python packages. Also modify prompt-compressor.yaml to require at least one rule with `upperTokenLimit: -1` by adding a `contains` constraint and `minContains: 1`, enforcing a catch-all fallback rule.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refers to - #1732
Purpose
Support more reliable builder-side handling of Python
pipPackagepolicies and align generated Python executor artifacts with the renamed SDK package. This is needed so Python policies referenced through direct/VCS specs, short URLs, and partial versions resolve deterministically and the generated runtime uses the updated SDK name consistently.Goals
Improve Python policy discovery and lockfile generation in the gateway builder.
Ensure pip-based policies are matched back to the correct manifest entry using the original pip spec.
Rename the Python SDK package from
wso2_gateway_policy_sdktoapip_sdk_coreand update generator/runtime expectations accordingly.Approach
Extended builder discovery to parse indexed and VCS pip specs, support short Git-style URLs, resolve major/minor-only refs to exact versions, and preserve the resolved exact pip spec in the generated manifest.
Updated manifest generation to prefer matching Python pip policies by the original
pipPackagevalue when multiple candidates share the same policy name.Updated the policy engine generator, SDK package layout, python-executor import resolution, and related tests to use
apip_sdk_coreinstead ofwso2_gateway_policy_sdk.Documentation
N/A - this PR mainly updates internal builder/runtime packaging behavior and test coverage; no product-facing documentation changes were made as part of this scope.
Automation tests
Security checks
Samples
N/A - no new long-lived samples are part of the final scope being highlighted in this PR.
Related PRs
N/A