feat: add phase 4 models - #24
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df3a25e990
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Adds new Pydantic model definitions for additional Alteryx Server API surfaces (“phase 4”), expanding the client’s typed request/response layer and re-exporting them from models.__init__ for easier consumption.
Changes:
- Added Collection models + request payload models (including permission-flattening
model_dump()helpers). - Added Credential models + request payload models.
- Added (currently schema-less) ServerInfo/ServerSettings response models and exported all new models in
models/__init__.py.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/alteryx_server_py/models/server.py |
Introduces ServerInfo/ServerSettings models (currently permissive “extra allow”). |
src/alteryx_server_py/models/credentials.py |
Adds credential response + create/update/share request models. |
src/alteryx_server_py/models/collections.py |
Adds collection response + create/update/share request models with permission flattening for form-encoded contracts. |
src/alteryx_server_py/models/__init__.py |
Re-exports the new models for public package access. |
Merge activity
|
|
Warning Review limit reached
More reviews will be available in 30 minutes and 9 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughIntroduces ChangesModel Refactor: PermissiveApiModel, Enums, and Permission Requests
CI Workflow Trigger Update
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7feb52e59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 51
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/alteryx_server_py/resources/schedules.py (1)
277-522: 🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy liftSignificant code duplication between sync and async classes.
AsyncScheduleResourceduplicates ~95% ofScheduleResource's logic, differing only in async/await keywords. This violates DRY and creates maintenance risk—business logic changes must be applied to both classes consistently.Consider refactoring to reduce duplication:
- Extract shared logic into a base class with template methods
- Use a mixin pattern for common validation/serialization logic
- Document clearly if both classes must be maintained independently
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/alteryx_server_py/resources/schedules.py` around lines 277 - 522, The AsyncScheduleResource class duplicates nearly all the logic from ScheduleResource, with the only differences being async/await keywords. Refactor this duplication by extracting the shared business logic into a common base class (such as a _BaseScheduleLogic) that contains the parameter validation, request building, and response parsing for all schedule operations (list, get, create, update, delete, enable, disable methods). Then have both ScheduleResource and AsyncScheduleResource inherit from or compose this base class, with each providing only their respective synchronous or asynchronous transport layer implementation. This ensures business logic changes only need to be made in one place, reducing maintenance risk and keeping the two classes in sync..github/copilot-instructions.md (1)
23-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove duplicate line.
Lines 23 and 24 contain identical text about using clear, consistent imports. One should be removed.
🔧 Proposed fix
- **Organize code into clearly separated modules**, grouped by feature or responsibility. For agents this looks like: - `agent.py` - Main agent definition and execution logic - `tools.py` - Tool functions used by the agent - `prompts.py` - System prompts - **Use clear, consistent imports** (prefer relative imports within packages). -- **Use clear, consistent imports** (prefer relative imports within packages). - **Use python_dotenv and load_env()** for environment variables.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/copilot-instructions.md around lines 23 - 24, Remove the duplicate line about using clear, consistent imports in the copilot-instructions.md file. Lines 23 and 24 both contain the identical text "Use clear, consistent imports (prefer relative imports within packages)." Delete one of these duplicate entries to eliminate the redundancy..github/workflows/branch-updates.yml (1)
42-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent matrix variable name.
Line 42 uses
poetry-version: ["1.8.2"]in theuv_targeted_testsjob, but line 14 in theproject_lintingjob usesuv-version. Since both jobs use uv (not poetry), the variable should be consistently nameduv-version.♻️ Proposed fix for consistency
matrix: python-version: ["3.10"] - poetry-version: ["1.8.2"] + uv-version: ["1.8.2"] os: [ubuntu-22.04 ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/branch-updates.yml at line 42, The matrix variable in the uv_targeted_tests job is named poetry-version but should be named uv-version to match the naming convention used in the project_linting job on line 14. Since both jobs use uv and not poetry, change the matrix variable name from poetry-version to uv-version in the uv_targeted_tests job to ensure consistency across the workflow file.hatch.toml (1)
9-10:⚠️ Potential issue | 🔴 CriticalRemove static version from pyproject.toml or reconfigure hatch-vcs.
The
[metadata.hooks.vcs]section is empty and provides no functional value if no dynamic metadata fields are defined. However, the more critical issue is the configuration mismatch:pyproject.tomldeclares a static version (version = "0.2.0"), buthatch.tomlconfigures hatch-vcs withsource = "vcs"to derive the version from git tags. To use hatch-vcs for dynamic versioning, adddynamic = ["version"]to[project]inpyproject.tomland remove the staticversion = "0.2.0"line. Alternatively, if static versioning is intended, remove the vcs source configuration and the hatch-vcs dependency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hatch.toml` around lines 9 - 10, The `[metadata.hooks.vcs]` section in hatch.toml is empty and there is a configuration mismatch: pyproject.toml declares a static version while hatch.toml is configured for hatch-vcs to derive the version from git tags. Choose one approach: either remove the `[metadata.hooks.vcs]` section from hatch.toml and the hatch-vcs dependency if you want to keep static versioning in pyproject.toml, or update pyproject.toml to add `dynamic = ["version"]` to the `[project]` section and remove the static `version = "0.2.0"` declaration to enable dynamic versioning with hatch-vcs. Ensure both configuration files are aligned with your intended versioning strategy.tests/unit/test_client_get_workflows_models.py (1)
22-72: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a failure-path test for
workflows.list().This module currently validates only successful payload parsing. Please add at least one negative case (e.g., invalid payload shape or request exception) so this suite includes expected, edge, and failure coverage.
As per coding guidelines,
**/tests/**/*.pymust include at least one expected, one edge, and one failure case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_client_get_workflows_models.py` around lines 22 - 72, The test file currently only includes successful payload parsing scenarios for client.workflows.list(). Add at least one failure-path test case to the module that validates how the code handles invalid payloads or request exceptions. This test should verify error handling, such as testing when an invalid payload shape is provided or when the _request method raises an exception, ensuring the module meets the coding guideline requirement of including expected, edge, and failure test coverage.Source: Coding guidelines
src/alteryx_server_py/resources/jobs.py (1)
170-183:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBinary output handling can still corrupt or mis-type downloaded artifacts.
get_outputassumes it may receive raw response bytes, but_requestcurrently normalizes success responses through_process_response(which returnsresponse.textfor non-JSON bodies). That means this path can round-trip binary payloads through text (lossy for non-UTF8), and the finalcast(bytes, response)can return non-bytes at runtime.Please add a true raw-response path for output downloads (or handle this endpoint outside
_process_response) and fail fast on unexpected response types instead of casting.Also applies to: 442-455
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/alteryx_server_py/resources/jobs.py` around lines 170 - 183, The get_output method assumes it receives raw response bytes but the _request method normalizes responses through _process_response, which converts non-JSON bodies to text via response.text. This causes binary data to be corrupted if it's not UTF-8 compatible. Additionally, the final cast(bytes, response) is unsafe and doesn't validate types at runtime. Either add a raw-response parameter to bypass _process_response normalization for this endpoint, or handle the output download outside of _process_response entirely. Replace all cast operations with explicit type checks that raise a clear error if the response type is unexpected, rather than silently casting potentially incorrect types.tests/unit/test_env_loading.py (1)
4-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a missing-env failure test for
AlteryxClient.from_env().This file currently tests success and override flows only. Please add a failure case where a required variable is absent and assert
ConfigurationError.As per coding guidelines, "
**/tests/**/*.py: Always create Pytest unit tests for new features ... Include at least 1 test for expected use, 1 edge case, and 1 failure case in test coverage."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_env_loading.py` around lines 4 - 20, The test_env_loading function currently only tests the success case where all environment variables are set and the override case where parameters are provided directly, but it lacks a failure case. Add a new test function that uses monkeypatch to unset one or more of the required environment variables (ALTERYX_BASE_URL, ALTERYX_CLIENT_ID, or ALTERYX_CLIENT_SECRET), then call AlteryxClient.from_env() and assert that it raises a ConfigurationError to cover the failure case requirement from the coding guidelines.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/copilot-instructions.md:
- Line 7: The heading "### 🌳 Git & PR Management (Graphite)" needs to be
surrounded by blank lines to follow Markdown formatting best practices. Add one
blank line before this heading and one blank line after it to improve
readability and formatting consistency.
In @.github/workflows/branch-updates.yml:
- Line 14: The workflow defines a matrix variable `uv-version` with value
`["1.8.2"]` but it is never used because the `with.uv-version` parameter in the
setup-uv action (lines 25-26) is commented out. To fix this, uncomment the
`with.uv-version` parameter lines so that the setup-uv action receives the
matrix-provided uv version value instead of using its default. This will ensure
the matrix variable definition is actually utilized in the workflow.
In @.github/workflows/python-package.yml:
- Line 15: Remove the "3.8" entry from the python-version matrix array in the
workflow file. Python 3.8 is not compatible with the project dependencies since
pydantic requires >= 3.9 and the project itself declares requires-python >=
3.10. Update the matrix to only include 3.9, 3.10, 3.11, and 3.12.
- Line 11: The job named poetry_full_matrix_test is misleading because the
workflow exclusively uses uv (installing it on line 25 and running uv sync on
line 27) instead of Poetry. Rename the job from poetry_full_matrix_test to
uv_full_matrix_test to accurately reflect the actual tool being used in the
workflow.
- Line 16: The matrix variable defined as poetry-version is misaligned with the
tool actually used in the workflow, which is uv. Either rename the matrix
variable poetry-version to uv-version to accurately reflect the tool being
configured, or remove it entirely if it is not referenced anywhere in the
workflow steps. Ensure the variable name matches the actual dependency
management tool being used.
In @.vscode/mcp.json:
- Around line 2-10: The .vscode/mcp.json configuration references the "gt"
command which requires Graphite CLI to be installed, but this dependency is not
documented anywhere for developers. Add documentation to the README or
devcontainer setup file that explicitly states the requirement for Graphite CLI
version 1.6.7 or higher and its `gt mcp` command support. Include this in either
the Environment Setup section or create a new Development Dependencies section
that clearly outlines this prerequisite for using the Graphite MCP server
configuration.
In `@pyproject.toml`:
- Around line 33-34: The pyproject.toml has an inconsistent versioning
configuration with hatch-vcs. Currently it has a hardcoded version field in the
[project] section while hatch.toml is configured for VCS-based versioning. To
fix this, add "version" to the dynamic list in the [project] section of
pyproject.toml and remove the hardcoded version = "0.2.0" line from the
[project] section, allowing hatch-vcs to manage the version dynamically from
version control instead.
In `@scratch.py`:
- Line 54: The logging.config.fileConfig call on line 54 executes during module
import without error handling, which causes the module to fail to import if the
logging.conf file is missing or if the current working directory has changed.
Wrap the logging.config.fileConfig call in a try-except block to handle the case
where the file does not exist or cannot be loaded, and log an appropriate
warning or info message instead of crashing. This will allow the module to be
imported successfully even when the logging configuration file is unavailable.
- Line 71: The authenticate method (and the other methods at the specified line
ranges) are missing proper Google-style docstrings or contain placeholder text.
Add a complete Google-style docstring to the authenticate method and the other
affected methods, ensuring each docstring includes a concise one-line summary of
what the method does, an Args section listing all parameters with their types
and descriptions, and a Returns section documenting the return type and what is
returned. Follow the Google docstring format strictly as required by the
repository coding guidelines.
- Around line 160-171: The method `_prepare_workflow_data` returns a tuple
containing two dictionaries, but in `post_publish_workflow` the code is
assigning this tuple directly on line 237 and then passing it as `data=...` on
line 263, which expects a single mapping rather than a tuple. Unpack the tuple
returned from `_prepare_workflow_data` into its two component dictionaries, and
use the appropriate dictionary (or merge them as needed) to pass as the `data`
parameter to the request call in `post_publish_workflow`.
- Around line 82-87: The authenticate() method call at line 97 does not validate
whether authentication succeeded before proceeding with subsequent operations,
which could lead to requests being made with an invalid or missing token.
Additionally, line 86 uses auth_data.get("expires_in") without a default value
or validation, which could result in None being added to the timestamp. Fix this
by checking the return value of the authenticate() method call and handling
failures explicitly (either by raising an exception or returning early), and
provide a safe default value or add validation when calculating
self.token_expiry to ensure "expires_in" is present and is a valid number before
performing the arithmetic operation.
- Around line 143-147: The code in the `_post` method creates an alias to
`self.headers` on line 143 and then mutates it by calling `pop("Content-Type",
None)` on line 146. Since this is an alias and not a copy, the shared
`self.headers` object is permanently modified after the first call, removing the
Content-Type header for all subsequent requests. Fix this by creating a shallow
copy of `self.headers` instead of aliasing it, so that the removal of the
Content-Type header only affects the local headers variable used for this
specific request and does not mutate the shared instance.
- Around line 73-81: The GalleryClient class has three HTTP requests (two POST
requests and one GET request) that lack timeout parameters, which can cause
threads to hang indefinitely during network issues. Add explicit timeout
parameters to all three requests.request() calls: the POST request to the OAuth
token endpoint at lines 73-81, the GET request around lines 112-116, and the
POST request around lines 149-154. Include a timeout keyword argument with an
appropriate timeout value (such as 30 seconds) to each requests.request()
invocation to ensure requests fail gracefully instead of hanging.
In `@src/alteryx_server_py/async_client.py`:
- Around line 208-257: Define private cache attributes (_jobs, _schedules,
_users, _user_groups) as instance variables in the __init__ method with proper
type annotations and initialize them to None. Then update the return type
annotations in the jobs, schedules, users, and user_groups properties from the
generic object type to their specific concrete types (AsyncJobResource,
AsyncScheduleResource, AsyncUserResource, AsyncUserGroupResource respectively).
Finally, simplify each property's logic to check if the cached attribute is None
instead of using hasattr, removing the redundant attribute existence check.
In `@src/alteryx_server_py/client.py`:
- Around line 116-117: The `json_data` parameter in the client method at line
116 is typed as `Optional[Dict[str, Any]]`, but the `add_users` method in the
user_groups resource passes a list payload instead. Broaden the type annotation
for the `json_data` parameter to accept both dictionary and list types using a
Union type, so it can accommodate the actual payloads being sent by resource
implementations like the list payload from `add_users`.
- Around line 223-273: The return type annotations for the property accessors
jobs, schedules, users, and user_groups are all using the generic object type
instead of their concrete resource classes. Update the return type annotation
for each property to use the specific resource class it returns: change the jobs
property to return JobResource, schedules property to return ScheduleResource,
users property to return UserResource, and user_groups property to return
UserGroupResource. This will improve type safety and enable better static
analysis.
In `@src/alteryx_server_py/models/credentials.py`:
- Line 30: Modify the ConfigDict configuration in credentials.py (at the
model_config line with populate_by_name=True) to include extra="forbid"
parameter to enforce strict validation and reject unknown fields. Apply this
same change to all 18 request models throughout the codebase, including
WorkflowUploadRequest, UserCreateRequest, ScheduleCreateRequest, JobRunRequest,
CollectionCreateRequest and all other similar request models by adding
extra="forbid" to their respective ConfigDict calls to ensure client-side errors
like field name typos are caught rather than silently ignored.
In `@src/alteryx_server_py/models/jobs.py`:
- Line 93: The JobRunRequest model's ConfigDict on line 93 currently only
includes populate_by_name=True but should also add extra="forbid" to strictly
validate and reject unexpected fields. Update the ConfigDict initialization to
include extra="forbid" alongside the existing populate_by_name=True parameter so
that Pydantic will reject requests with unexpected keys rather than silently
ignoring them.
In `@src/alteryx_server_py/models/users.py`:
- Line 62: Add `extra="forbid"` parameter to all four ConfigDict declarations
(at lines 62, 84, 125, and 141 in the user request schemas) to enforce strict
validation and reject unknown fields. Modify each `model_config =
ConfigDict(populate_by_name=True)` statement by adding `extra="forbid"` to the
ConfigDict parameters, ensuring that misspelled or unknown fields are caught
during validation rather than silently ignored.
In `@src/alteryx_server_py/models/workflows.py`:
- Line 72: The workflow request and response models lack strict field validation
for unknown fields. Add `extra="forbid"` to the ConfigDict configuration in all
three model definitions (at lines 72, 95, and 136 in the models/workflows.py
file) to prevent silent acceptance of unknown fields and align with the
BaseApiModel pattern used elsewhere in the codebase. Modify each `model_config =
ConfigDict(populate_by_name=True)` statement to include the `extra="forbid"`
parameter alongside the existing populate_by_name setting.
In `@src/alteryx_server_py/resources/jobs.py`:
- Around line 185-186: Replace the brittle string parsing approach in the
exception handling (where you currently check if "not found" or "404" is in
str(e).lower()) with direct catching of NotFoundError exception type. Instead of
examining the string representation of the exception, catch NotFoundError
explicitly and then raise JobNotFoundError from it, following the same pattern
already used in AsyncJobResource.cancel method. Apply this fix to all locations
where you're currently doing string parsing for "not found"/"404" errors
(including the block at lines 457-458).
In `@src/alteryx_server_py/resources/schedules.py`:
- Around line 349-398: The async create method is missing validation for the
frequency parameter before converting it with ScheduleFrequency(frequency). Add
validation logic before the ScheduleCreateRequest instantiation to check if the
provided frequency value is valid, and raise a descriptive ValueError with
helpful context if it is not. This validation should match the same approach
applied to the sync version of the create method to ensure consistency across
both implementations.
- Around line 317-324: The async method's response parsing logic in the section
handling list responses, dictionaries with "schedules" key, and plain
dictionaries silently returns an empty list when the response doesn't match any
expected shape. Review the fix that was applied to the sync version of this
logic (lines 70-77) and apply the identical error handling or logging approach
to this async version. Instead of falling through to return an empty list when
response shape doesn't match expected patterns, implement the same error
handling strategy used in the sync version to prevent silent API failures.
- Around line 70-77: The response normalization logic that handles multiple
response shapes (list, dict with "schedules" key, and plain dict) lacks
documentation explaining why this complexity exists. Add an inline # Reason:
comment before the if isinstance(response, list) block that explains why the API
returns different response shapes (such as support for different API versions,
pagination patterns, or single-item responses) and why all shapes need to be
normalized to a list of Schedule.model_validate objects.
- Around line 70-77: The code currently silently returns an empty list when the
API response does not match any of the expected formats (list, dict with
"schedules" key, or plain dict). Instead of the silent fallthrough with `return
[]`, add a warning log or raise a descriptive error before that line to alert
when an unrecognized response shape is encountered. This will help catch API
contract changes or unexpected behavior, making the issue visible rather than
hidden. Reference the existing `Schedule.model_validate()` calls and the
response type checks to understand the expected formats, then add appropriate
logging or exception handling for the unrecognized case.
- Around line 102-152: The create method converts the frequency parameter to
ScheduleFrequency enum without error handling, which will raise a generic
ValueError if an invalid frequency string is provided. Add a try-catch block
around the ScheduleFrequency(frequency) conversion in the create method to catch
the ValueError, and re-raise it with a descriptive error message that lists the
valid frequency options. Alternatively, change the frequency parameter type hint
from str to Literal with the valid frequency values to provide better API
guidance and type safety.
In `@src/alteryx_server_py/resources/user_groups.py`:
- Around line 66-73: The response normalization pattern that handles list,
wrapped dict, single dict, and empty responses is duplicated across users.py and
user_groups.py (appearing in multiple methods in each file). Create a shared
helper function named _normalize_list_response in _base.py that accepts response
and an optional wrapper_key parameter, then returns the normalized list of
items. Replace all occurrences of the duplicated isinstance checks (the blocks
that check for list, dict with wrapper key, single dict, and empty cases) with
calls to this helper function, passing the appropriate wrapper key name such as
"users" or "userGroups", and then apply UserGroup.model_validate to each
normalized item.
- Around line 89-96: The exception handlers that translate NotFoundError to
UserGroupNotFoundError are not preserving the exception chain, which makes
debugging difficult. For each exception handler in the file that catches
NotFoundError and raises UserGroupNotFoundError (and any other exception
translations mentioned at lines 95-96, 178-179, 198-199, 223-224, 244-245),
capture the caught exception using an as clause and use exception chaining
syntax (raise ... from e) when re-raising the custom exception. This ensures the
original exception context is preserved for debugging purposes.
- Around line 114-122: Add validation error handling for the role conversion in
the create method (where UserRole(role) is called) and also apply the same
validation pattern to the update method at line 159. Instead of letting
UserRole(role) potentially raise a confusing ValueError for invalid role
strings, validate the role parameter first using the same pattern that was
applied in users.py, providing clear error messages if the role is invalid. This
should wrap the role conversion logic with proper error handling in both
locations where UserRole is instantiated with user-provided input.
In `@src/alteryx_server_py/resources/users.py`:
- Around line 217-244: The get_assets method returns List[Dict[str, Any]] which
lacks type safety. If asset structures are consistent across resource types,
create a Pydantic model to represent the asset structure (e.g., an Asset model)
and update the get_assets method's return type annotation to use List[Asset]
instead. Then parse the response data into instances of this model to provide
proper type validation and type safety for callers of the method.
- Around line 89-96: The exception handling block that catches NotFoundError and
raises UserNotFoundError does not use exception chaining. Modify the except
clause to capture the caught NotFoundError into a variable (using 'as e'), and
then change the raise statement to use 'raise UserNotFoundError(user_id) from e'
to preserve the original exception chain and maintain the full traceback for
debugging. Apply this same pattern of exception chaining to all other locations
in the file where NotFoundError is caught and translated into custom exception
types.
- Around line 170-181: The line `user_role = UserRole(role) if role else None`
in the update method can raise a ValueError if an invalid role string is
provided. Add error handling around the UserRole enum conversion to catch
ValueError exceptions, validate the role parameter properly, and return an
appropriate HTTP error response (such as a 400 Bad Request) with a clear message
indicating the invalid role value, similar to how this should be handled in the
create method.
- Around line 122-129: The UserRole(role) conversion in the UserCreateRequest
construction may raise a generic ValueError when an invalid role string is
provided, which offers poor guidance to API users. Add explicit validation for
the role parameter before passing it to UserRole, catch any ValueError
exceptions from the conversion, and re-raise with a clearer error message that
indicates the valid role values. Alternatively, validate the role input against
known valid UserRole values at the start of the function and raise a descriptive
validation error before attempting the conversion.
- Around line 66-73: Add inline `# Reason:` comments to each branch in the
response normalization logic within the function that contains the isinstance
checks for list, dict with "users" key, and single dict. Each comment should
explain why that particular API response shape is being handled (e.g., why the
API might return a list, why it might return a dict with a "users" key, and why
it might return a single dict). This helps future maintainers understand the API
contract and the necessity for this branching logic when the API behavior
evolves.
In `@tasks.md`:
- Around line 334-336: Remove the stray bullet point line "- Complete migration
guide" that appears after the Version metadata in the tasks.md footer section.
This line is dangling content that doesn't belong in the Last Updated/Version
metadata footer and should be deleted to clean up the formatting.
- Line 43: The tasks.md file contains corrupted encoding glyphs (mojibake
characters) at lines 43, 86, 87, 88, 140, and 278 where arrow separators or
directional indicators should be. Replace each instance of the corrupted glyph
sequence (the garbled characters like ├ö├Â┬úÔö£├é etc.) with an appropriate
arrow character or separator such as `→` or another directional indicator that
makes the task description clear and readable. Focus on the lines that contain
task descriptions with path movements to ensure the instructions are
unambiguous.
In `@tests/archive_test_gallery_connection.py`:
- Line 6: The import statement on line 6 references `AlteryxGallery` but the
actual module directory is named `archive_AlteryxGallery`, causing a runtime
import failure. Additionally, the `__init__.py` file in that directory does not
export `AlteryxGalleryAPI`. Fix this by changing the import from `from
AlteryxGallery import AlteryxGalleryAPI` to use the correct module name
`archive_AlteryxGallery`, and ensure the `__init__.py` file exports the
`AlteryxGalleryAPI` class so it can be imported properly.
In `@tests/integration/test_live_client.py`:
- Around line 39-46: The pytest.raises context manager in the test is catching
both AuthenticationError and the broad Exception class, which is too permissive
and could mask unrelated runtime errors. Replace the exception tuple in
pytest.raises with only the specific authentication failure exception type(s)
that should actually occur when AlteryxClient is instantiated with invalid
credentials and client.workflows.list() is called. This ensures the test
validates the intended authentication failure rather than any arbitrary
exception.
- Around line 71-74: The test code only executes the workflow validation block
when the workflows list is non-empty, but provides no handling for the case when
workflows is empty, causing the test to pass silently without actually
validating anything. Add an else clause after the if workflows: conditional
block that calls pytest.skip() with a descriptive message explaining that the
test is being skipped due to no workflows being available. This ensures the test
intent remains truthful and prevents false-positive passes.
In `@tests/test_client.py`:
- Around line 16-45: The test functions test_successful_initialization,
test_base_url_trailing_slash_added, test_missing_client_id_raises,
test_missing_client_secret_raises, and test_missing_base_url_raises are missing
return type annotations and use non-standard docstring formatting. Add `-> None`
return type annotation to each function signature and convert their docstrings
from simple format to Google-style format with a brief summary, empty Args
section (since test functions have no parameters), and no Returns section needed
since they return None. This aligns with the project's PEP8 and Google-style
docstring conventions.
In `@tests/unit/models/test_collections_model.py`:
- Around line 73-84: Update the docstring for the method
test_user_permission_update_request_requires_expiration_date to accurately
reflect what the test validates. The current docstring states the test requires
an expiry timestamp, but the test only validates serialization behavior when the
field is provided. Clarify that the docstring describes testing the correct
serialization and aliasing of the expiration_date field (such as the camelCase
conversion to expirationDate) rather than claiming it validates the field is
required.
In `@tests/unit/models/test_schedules_model.py`:
- Around line 56-69: The test_schedule_rejects_unknown_fields method catches a
broad Exception type, which could mask unexpected errors. Replace the generic
Exception with the specific pydantic.ValidationError that Pydantic v2 raises
when extra="forbid" is configured on the Schedule model. You will need to import
ValidationError from the pydantic module at the top of the test file, then
update the pytest.raises() call to use ValidationError instead of Exception.
In `@tests/unit/models/test_users_model.py`:
- Around line 56-66: In the test_user_rejects_unknown_fields method, replace the
broad Exception catch with the specific pydantic ValidationError. Import
ValidationError from pydantic at the top of the file, then change
pytest.raises(Exception) to pytest.raises(ValidationError) to directly test the
expected Pydantic error behavior when unknown fields are provided in strict
mode.
- Around line 143-151: In the test_group_rejects_unknown_fields method, replace
the broad Exception catch with the specific pydantic.ValidationError. Import
ValidationError from pydantic at the top of the file and update the
pytest.raises call to catch ValidationError instead of Exception, since
UserGroup extends BaseApiModel with extra="forbid" which will raise this
specific error when unknown fields are encountered.
In `@tests/unit/models/test_workflows_model.py`:
- Around line 3-22: The test function test_workflow_parses_reduced_view_payload
uses the datetime class in the isinstance assertion but the datetime module is
not imported at the top of the file. Add an import statement for datetime from
the datetime module alongside the existing import statement for the Workflow
class at the beginning of the file.
In `@tests/unit/test_client_get_workflows_models.py`:
- Around line 43-45: The monkeypatch stub for the `_request` method is too
permissive and does not validate the HTTP method or endpoint being called,
allowing regressions to go undetected. Replace the lambda function in the
monkeypatch.setattr call with a proper stub function that asserts the expected
HTTP method and path arguments (the first and second positional arguments passed
to _request) before returning the payload. Apply the same approach to all other
similar monkeypatch stubs in the test file where _request is being mocked to
ensure the correct contract is being enforced.
In `@tests/unit/test_job_resource.py`:
- Around line 193-198: The timeout parameter in the run_and_wait call is set to
0, which causes the function to timeout immediately without executing any
polling iterations, so the intended polling behavior is never tested. Replace
the timeout=0 argument with a small positive timeout value (such as 0.1 or 1) to
ensure the function enters and completes at least one poll cycle before timing
out, thereby properly exercising the never-finishes polling behavior that this
test is meant to verify.
- Around line 116-127: The test_get_job_output test currently uses
ASCII-compatible bytes which could pass even if the implementation incorrectly
decodes and re-encodes the output. Add a separate test case or additional
assertion within test_get_job_output that uses non-UTF8 bytes (for example,
bytes with values that are not valid UTF8 sequences) and verify that the output
returned by async_client.jobs.get_output matches the input bytes exactly through
direct equality assertion to ensure true byte-level preservation without any
text encoding/decoding.
In `@tests/unit/test_schedule_resource.py`:
- Around line 29-38: The async_client fixture creates an AsyncAlteryxClient
which initializes an internal httpx.AsyncClient but never closes it, causing
resource leaks. Convert the async_client fixture from a simple return-based
fixture to a yield-based fixture that properly cleans up resources. Yield the
client after creation, then close the underlying async client in the teardown
section after the yield statement to ensure the HTTP connection is properly
closed after each test completes.
In `@tests/unit/test_workflows.py`:
- Around line 30-48: The test_get_workflows function only validates the happy
path where client.workflows.list() successfully returns a workflow. Add at least
two additional test functions to improve coverage: one test function that
verifies the edge case of an empty response from client.workflows.list() (where
the payload is an empty list), and another test function that validates the
failure case where the underlying _request call raises an exception. Use
monkeypatch in both additional tests to mock the appropriate behavior and assert
that the results are handled correctly in each scenario.
- Around line 16-31: Add type hints and Google-style docstrings to the client
fixture and test_get_workflows function. For the client fixture, add a return
type annotation indicating it returns AlteryxClient. For test_get_workflows, add
type hints for the client and monkeypatch parameters and a return type of None.
Replace the current minimal docstrings with Google-style format for both
functions, including a brief summary line, an Args section documenting each
parameter, and a Returns section describing what is returned (including None for
test functions).
---
Outside diff comments:
In @.github/copilot-instructions.md:
- Around line 23-24: Remove the duplicate line about using clear, consistent
imports in the copilot-instructions.md file. Lines 23 and 24 both contain the
identical text "Use clear, consistent imports (prefer relative imports within
packages)." Delete one of these duplicate entries to eliminate the redundancy.
In @.github/workflows/branch-updates.yml:
- Line 42: The matrix variable in the uv_targeted_tests job is named
poetry-version but should be named uv-version to match the naming convention
used in the project_linting job on line 14. Since both jobs use uv and not
poetry, change the matrix variable name from poetry-version to uv-version in the
uv_targeted_tests job to ensure consistency across the workflow file.
In `@hatch.toml`:
- Around line 9-10: The `[metadata.hooks.vcs]` section in hatch.toml is empty
and there is a configuration mismatch: pyproject.toml declares a static version
while hatch.toml is configured for hatch-vcs to derive the version from git
tags. Choose one approach: either remove the `[metadata.hooks.vcs]` section from
hatch.toml and the hatch-vcs dependency if you want to keep static versioning in
pyproject.toml, or update pyproject.toml to add `dynamic = ["version"]` to the
`[project]` section and remove the static `version = "0.2.0"` declaration to
enable dynamic versioning with hatch-vcs. Ensure both configuration files are
aligned with your intended versioning strategy.
In `@src/alteryx_server_py/resources/jobs.py`:
- Around line 170-183: The get_output method assumes it receives raw response
bytes but the _request method normalizes responses through _process_response,
which converts non-JSON bodies to text via response.text. This causes binary
data to be corrupted if it's not UTF-8 compatible. Additionally, the final
cast(bytes, response) is unsafe and doesn't validate types at runtime. Either
add a raw-response parameter to bypass _process_response normalization for this
endpoint, or handle the output download outside of _process_response entirely.
Replace all cast operations with explicit type checks that raise a clear error
if the response type is unexpected, rather than silently casting potentially
incorrect types.
In `@src/alteryx_server_py/resources/schedules.py`:
- Around line 277-522: The AsyncScheduleResource class duplicates nearly all the
logic from ScheduleResource, with the only differences being async/await
keywords. Refactor this duplication by extracting the shared business logic into
a common base class (such as a _BaseScheduleLogic) that contains the parameter
validation, request building, and response parsing for all schedule operations
(list, get, create, update, delete, enable, disable methods). Then have both
ScheduleResource and AsyncScheduleResource inherit from or compose this base
class, with each providing only their respective synchronous or asynchronous
transport layer implementation. This ensures business logic changes only need to
be made in one place, reducing maintenance risk and keeping the two classes in
sync.
In `@tests/unit/test_client_get_workflows_models.py`:
- Around line 22-72: The test file currently only includes successful payload
parsing scenarios for client.workflows.list(). Add at least one failure-path
test case to the module that validates how the code handles invalid payloads or
request exceptions. This test should verify error handling, such as testing when
an invalid payload shape is provided or when the _request method raises an
exception, ensuring the module meets the coding guideline requirement of
including expected, edge, and failure test coverage.
In `@tests/unit/test_env_loading.py`:
- Around line 4-20: The test_env_loading function currently only tests the
success case where all environment variables are set and the override case where
parameters are provided directly, but it lacks a failure case. Add a new test
function that uses monkeypatch to unset one or more of the required environment
variables (ALTERYX_BASE_URL, ALTERYX_CLIENT_ID, or ALTERYX_CLIENT_SECRET), then
call AlteryxClient.from_env() and assert that it raises a ConfigurationError to
cover the failure case requirement from the coding guidelines.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: ad408c19-b85a-4b9b-9c29-8f65a670fef3
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (66)
.github/copilot-instructions.md.github/prompts/execute-prp.prompt.md.github/prompts/generate-prp.prompt.md.github/workflows/branch-updates.yml.github/workflows/python-package.yml.vscode/mcp.json.vscode/settings.jsonhatch.tomlpyproject.tomlquick_test.pyscratch.pysrc/alteryx_gallery_api/client.pysrc/alteryx_gallery_api/exceptions.pysrc/alteryx_gallery_api/models/__init__.pysrc/alteryx_gallery_api/models/auth.pysrc/alteryx_gallery_api/models/base.pysrc/alteryx_gallery_api/models/common.pysrc/alteryx_gallery_api/models/workflows.pysrc/alteryx_server_py/__init__.pysrc/alteryx_server_py/_base_client.pysrc/alteryx_server_py/async_client.pysrc/alteryx_server_py/auth.pysrc/alteryx_server_py/client.pysrc/alteryx_server_py/config.pysrc/alteryx_server_py/exceptions.pysrc/alteryx_server_py/models/__init__.pysrc/alteryx_server_py/models/auth.pysrc/alteryx_server_py/models/base.pysrc/alteryx_server_py/models/collections.pysrc/alteryx_server_py/models/common.pysrc/alteryx_server_py/models/credentials.pysrc/alteryx_server_py/models/jobs.pysrc/alteryx_server_py/models/schedules.pysrc/alteryx_server_py/models/server.pysrc/alteryx_server_py/models/users.pysrc/alteryx_server_py/models/workflows.pysrc/alteryx_server_py/resources/__init__.pysrc/alteryx_server_py/resources/_base.pysrc/alteryx_server_py/resources/jobs.pysrc/alteryx_server_py/resources/schedules.pysrc/alteryx_server_py/resources/user_groups.pysrc/alteryx_server_py/resources/users.pysrc/alteryx_server_py/resources/workflows.pysrc/alteryx_server_py/utils/__init__.pysrc/alteryx_server_py/utils/file_utils.pysrc/alteryx_server_py/utils/pagination.pysrc/alteryx_server_py/utils/retry.pysrc/archive_AlteryxGallery/AlteryxGalleryAPI.pytasks.mdtests/archive_test_gallery_connection.pytests/integration/test_live_client.pytests/test_client.pytests/unit/models/test_collections_model.pytests/unit/models/test_credentials_model.pytests/unit/models/test_schedules_model.pytests/unit/models/test_users_model.pytests/unit/models/test_workflows_model.pytests/unit/test_client_get_workflows_models.pytests/unit/test_env_loading.pytests/unit/test_exceptions.pytests/unit/test_job_resource.pytests/unit/test_retry.pytests/unit/test_schedule_resource.pytests/unit/test_user_group_resource.pytests/unit/test_user_resource.pytests/unit/test_workflows.py
💤 Files with no reviewable changes (2)
- src/alteryx_gallery_api/client.py
- src/archive_AlteryxGallery/AlteryxGalleryAPI.py
b779d88 to
b1c36cc
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b779d889c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1c36cc8e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
b1c36cc to
10f5c97
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/python-package.yml (1)
1-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit least-privilege
permissionsforGITHUB_TOKEN.The workflow currently relies on default token permissions, which are broader than needed.
Suggested hardening diff
name: Main Branch Validation +permissions: + contents: read + on:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/python-package.yml around lines 1 - 54, The workflow currently lacks explicit `permissions` configuration for `GITHUB_TOKEN`, which means it uses default permissions that are broader than necessary for security. Add a `permissions` section at the top level of the workflow (after the `on:` trigger configuration and before the `jobs:` section) that explicitly grants only the least-privilege permissions needed. For this workflow, you need `contents: read` permission to allow the checkout action to function in both the test and build jobs, while denying all other permissions that are not required.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/python-package.yml:
- Line 23: The actions/checkout steps at lines 23 and 39 are missing the
persist-credentials configuration, which leaves GitHub tokens exposed to
subsequent workflow steps. Add persist-credentials: false to both instances of
the actions/checkout@v6 step to disable persisted credentials and reduce the
token exposure risk. This should be added as a configuration parameter in each
checkout action usage.
- Around line 3-12: The workflow is missing a top-level concurrency
configuration that would prevent redundant CI runs from queuing up on repeated
pushes. Add a concurrency block at the workflow level (after the "on" trigger
section) that groups runs by the branch name for push events and by pull request
for pull_request events, with cancel-in-progress set to true so that any
previous runs for the same branch or pull request are automatically cancelled
when a new run is triggered.
- Around line 23-24: Replace all mutable GitHub Actions version tags with full
commit SHAs throughout the workflow file. This includes actions/checkout@v6,
actions/setup-python@v6, and any other action references (at lines 28, 39-40,
44, 50). For each action, replace the `@v<number>` tag with the full commit SHA
(e.g., `@abc123def456...`) to ensure reproducibility and prevent supply chain
drift from tag retargeting by maintainers. You can find the commit SHA for each
action version by checking the action's GitHub release page or tag history.
In `@src/alteryx_server_py/models/collections.py`:
- Around line 62-69: The model_dump method override lacks explicit type hints
and a complete Google-style docstring. Add type annotations to the method
signature for *args, **kwargs, and the return type (the return type should match
the parent class, typically Any or dict). Expand the docstring to include a
brief summary (already present), followed by an Args section documenting the
*args and **kwargs parameters, and a Returns section documenting what the method
returns, following Google-style docstring format for maintainability.
- Line 110: Remove the legacy `CollectionPermissionUpdateRequest` alias
assignment that maps to `CollectionGroupPermissionUpdateRequest` in the
collections.py models file. This alias causes validation bypass because
`CollectionGroupPermissionUpdateRequest` has an optional `expiration_date` field
while `CollectionUserPermissionUpdateRequest` requires it. Update the collection
resource methods to explicitly use `CollectionUserPermissionUpdateRequest` for
user permission updates and `CollectionGroupPermissionUpdateRequest` for group
permission updates, ensuring that the correct field requirements are enforced
based on the target update type.
In `@src/alteryx_server_py/models/workflows.py`:
- Around line 90-104: The legacy normalization branches in the
workflow_type_mapping and execution_mode handling lack explanatory inline
comments about why those specific defaults are chosen. Add inline # Reason:
comments before the assignments to normalized["workflowType"] (where it uses
workflow_type_mapping.get() with "Standard" as the fallback) and before the
assignment to normalized["executionMode"] (where it defaults to "Safe"). These
comments should explain the business rationale or reasoning for these specific
default values to aid future maintenance.
In `@tests/unit/models/test_credentials_model.py`:
- Around line 42-55: The existing tests test_user_share_request_serialization
and test_user_group_share_request_serialization only cover the happy path for
successful serialization. Add two new failure-path tests: one that attempts to
instantiate CredentialUserShareRequest without the required user_id parameter
and expects a ValidationError to be raised, and another that attempts to
instantiate CredentialUserGroupShareRequest without the required user_group_id
parameter and expects a ValidationError to be raised. These tests will ensure
the required-field contracts are properly validated and protect against missing
identifier cases.
In `@tests/unit/models/test_workflows_model.py`:
- Around line 64-76: Add a new test function after
test_workflow_preserves_standard_execution_mode that validates the failure case
for invalid executionMode coercion. Create a payload with an unsupported or
invalid executionMode value (e.g., a legacy or nonexistent value), pass it to
Workflow.model_validate(), and assert that the resulting wf.execution_mode.value
is normalized to "Safe" to ensure backward compatibility and safe defaults for
unexpected values.
---
Outside diff comments:
In @.github/workflows/python-package.yml:
- Around line 1-54: The workflow currently lacks explicit `permissions`
configuration for `GITHUB_TOKEN`, which means it uses default permissions that
are broader than necessary for security. Add a `permissions` section at the top
level of the workflow (after the `on:` trigger configuration and before the
`jobs:` section) that explicitly grants only the least-privilege permissions
needed. For this workflow, you need `contents: read` permission to allow the
checkout action to function in both the test and build jobs, while denying all
other permissions that are not required.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 6f15bf9a-ac2d-4812-9ed3-9b906f367c40
📒 Files selected for processing (11)
.github/workflows/python-package.ymlsrc/alteryx_server_py/models/__init__.pysrc/alteryx_server_py/models/base.pysrc/alteryx_server_py/models/collections.pysrc/alteryx_server_py/models/common.pysrc/alteryx_server_py/models/credentials.pysrc/alteryx_server_py/models/server.pysrc/alteryx_server_py/models/workflows.pytests/unit/models/test_collections_model.pytests/unit/models/test_credentials_model.pytests/unit/models/test_workflows_model.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/python-package.yml (1)
1-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit least-privilege
permissionsforGITHUB_TOKEN.The workflow currently relies on default token permissions, which are broader than needed.
Suggested hardening diff
name: Main Branch Validation +permissions: + contents: read + on:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/python-package.yml around lines 1 - 54, The workflow currently lacks explicit `permissions` configuration for `GITHUB_TOKEN`, which means it uses default permissions that are broader than necessary for security. Add a `permissions` section at the top level of the workflow (after the `on:` trigger configuration and before the `jobs:` section) that explicitly grants only the least-privilege permissions needed. For this workflow, you need `contents: read` permission to allow the checkout action to function in both the test and build jobs, while denying all other permissions that are not required.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/python-package.yml:
- Line 23: The actions/checkout steps at lines 23 and 39 are missing the
persist-credentials configuration, which leaves GitHub tokens exposed to
subsequent workflow steps. Add persist-credentials: false to both instances of
the actions/checkout@v6 step to disable persisted credentials and reduce the
token exposure risk. This should be added as a configuration parameter in each
checkout action usage.
- Around line 3-12: The workflow is missing a top-level concurrency
configuration that would prevent redundant CI runs from queuing up on repeated
pushes. Add a concurrency block at the workflow level (after the "on" trigger
section) that groups runs by the branch name for push events and by pull request
for pull_request events, with cancel-in-progress set to true so that any
previous runs for the same branch or pull request are automatically cancelled
when a new run is triggered.
- Around line 23-24: Replace all mutable GitHub Actions version tags with full
commit SHAs throughout the workflow file. This includes actions/checkout@v6,
actions/setup-python@v6, and any other action references (at lines 28, 39-40,
44, 50). For each action, replace the `@v<number>` tag with the full commit SHA
(e.g., `@abc123def456...`) to ensure reproducibility and prevent supply chain
drift from tag retargeting by maintainers. You can find the commit SHA for each
action version by checking the action's GitHub release page or tag history.
In `@src/alteryx_server_py/models/collections.py`:
- Around line 62-69: The model_dump method override lacks explicit type hints
and a complete Google-style docstring. Add type annotations to the method
signature for *args, **kwargs, and the return type (the return type should match
the parent class, typically Any or dict). Expand the docstring to include a
brief summary (already present), followed by an Args section documenting the
*args and **kwargs parameters, and a Returns section documenting what the method
returns, following Google-style docstring format for maintainability.
- Line 110: Remove the legacy `CollectionPermissionUpdateRequest` alias
assignment that maps to `CollectionGroupPermissionUpdateRequest` in the
collections.py models file. This alias causes validation bypass because
`CollectionGroupPermissionUpdateRequest` has an optional `expiration_date` field
while `CollectionUserPermissionUpdateRequest` requires it. Update the collection
resource methods to explicitly use `CollectionUserPermissionUpdateRequest` for
user permission updates and `CollectionGroupPermissionUpdateRequest` for group
permission updates, ensuring that the correct field requirements are enforced
based on the target update type.
In `@src/alteryx_server_py/models/workflows.py`:
- Around line 90-104: The legacy normalization branches in the
workflow_type_mapping and execution_mode handling lack explanatory inline
comments about why those specific defaults are chosen. Add inline # Reason:
comments before the assignments to normalized["workflowType"] (where it uses
workflow_type_mapping.get() with "Standard" as the fallback) and before the
assignment to normalized["executionMode"] (where it defaults to "Safe"). These
comments should explain the business rationale or reasoning for these specific
default values to aid future maintenance.
In `@tests/unit/models/test_credentials_model.py`:
- Around line 42-55: The existing tests test_user_share_request_serialization
and test_user_group_share_request_serialization only cover the happy path for
successful serialization. Add two new failure-path tests: one that attempts to
instantiate CredentialUserShareRequest without the required user_id parameter
and expects a ValidationError to be raised, and another that attempts to
instantiate CredentialUserGroupShareRequest without the required user_group_id
parameter and expects a ValidationError to be raised. These tests will ensure
the required-field contracts are properly validated and protect against missing
identifier cases.
In `@tests/unit/models/test_workflows_model.py`:
- Around line 64-76: Add a new test function after
test_workflow_preserves_standard_execution_mode that validates the failure case
for invalid executionMode coercion. Create a payload with an unsupported or
invalid executionMode value (e.g., a legacy or nonexistent value), pass it to
Workflow.model_validate(), and assert that the resulting wf.execution_mode.value
is normalized to "Safe" to ensure backward compatibility and safe defaults for
unexpected values.
---
Outside diff comments:
In @.github/workflows/python-package.yml:
- Around line 1-54: The workflow currently lacks explicit `permissions`
configuration for `GITHUB_TOKEN`, which means it uses default permissions that
are broader than necessary for security. Add a `permissions` section at the top
level of the workflow (after the `on:` trigger configuration and before the
`jobs:` section) that explicitly grants only the least-privilege permissions
needed. For this workflow, you need `contents: read` permission to allow the
checkout action to function in both the test and build jobs, while denying all
other permissions that are not required.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 6f15bf9a-ac2d-4812-9ed3-9b906f367c40
📒 Files selected for processing (11)
.github/workflows/python-package.ymlsrc/alteryx_server_py/models/__init__.pysrc/alteryx_server_py/models/base.pysrc/alteryx_server_py/models/collections.pysrc/alteryx_server_py/models/common.pysrc/alteryx_server_py/models/credentials.pysrc/alteryx_server_py/models/server.pysrc/alteryx_server_py/models/workflows.pytests/unit/models/test_collections_model.pytests/unit/models/test_credentials_model.pytests/unit/models/test_workflows_model.py
🛑 Comments failed to post (8)
.github/workflows/python-package.yml (3)
3-12: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add workflow-level concurrency to cancel superseded runs.
Without a concurrency group, repeated pushes/force-pushes can leave redundant CI runs competing for capacity.
Suggested resilience diff
on: push: @@ pull_request: @@ +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + jobs:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.on: push: branches: - main - master pull_request: branches: - main - master concurrency: group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true🧰 Tools
🪛 zizmor (1.25.2)
[warning] 3-11: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/python-package.yml around lines 3 - 12, The workflow is missing a top-level concurrency configuration that would prevent redundant CI runs from queuing up on repeated pushes. Add a concurrency block at the workflow level (after the "on" trigger section) that groups runs by the branch name for push events and by pull request for pull_request events, with cancel-in-progress set to true so that any previous runs for the same branch or pull request are automatically cancelled when a new run is triggered.Source: Linters/SAST tools
23-23:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDisable persisted checkout credentials in both jobs.
actions/checkoutshould setpersist-credentials: falseto reduce token exposure risk across later steps/artifacts.Suggested hardening diff
- - uses: actions/checkout@v6 + - uses: actions/checkout@v6 + with: + persist-credentials: false @@ - - uses: actions/checkout@v6 + - uses: actions/checkout@v6 + with: + persist-credentials: falseAlso applies to: 39-39
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 23-23: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 23-23: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/python-package.yml at line 23, The actions/checkout steps at lines 23 and 39 are missing the persist-credentials configuration, which leaves GitHub tokens exposed to subsequent workflow steps. Add persist-credentials: false to both instances of the actions/checkout@v6 step to disable persisted credentials and reduce the token exposure risk. This should be added as a configuration parameter in each checkout action usage.Source: Linters/SAST tools
23-24:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/python-package.ymlRepository: Sivivatu/AlteryxGalleryAPI
Length of output: 1884
Pin GitHub Actions to commit SHAs instead of mutable tags.
Mutable version tags like
@v6and@v7can be retargeted by maintainers, creating supply chain drift. Pin each action to a full commit SHA for reproducibility and security.Suggested hardening diff
- - uses: actions/checkout@v6 - - uses: actions/setup-python@v6 # TODO: Should this be used or replaced with UV's setup-uv action? + - uses: actions/checkout@<FULL_SHA> + - uses: actions/setup-python@<FULL_SHA> # TODO: Should this be used or replaced with UV's setup-uv action? @@ - - name: Install uv - uses: astral-sh/setup-uv@v7 + - name: Install uv + uses: astral-sh/setup-uv@<FULL_SHA> @@ - - uses: actions/checkout@v6 - - uses: actions/setup-python@v6 # TODO: Should this be used or replaced with UV's setup-uv action? + - uses: actions/checkout@<FULL_SHA> + - uses: actions/setup-python@<FULL_SHA> # TODO: Should this be used or replaced with UV's setup-uv action? @@ - - name: Install uv - uses: astral-sh/setup-uv@v7 + - name: Install uv + uses: astral-sh/setup-uv@<FULL_SHA> @@ - - name: Upload distribution artifacts - uses: actions/upload-artifact@v7 + - name: Upload distribution artifacts + uses: actions/upload-artifact@<FULL_SHA>Also applies to: 28, 39-40, 44, 50
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 23-23: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 23-23: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 24-24: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/python-package.yml around lines 23 - 24, Replace all mutable GitHub Actions version tags with full commit SHAs throughout the workflow file. This includes actions/checkout@v6, actions/setup-python@v6, and any other action references (at lines 28, 39-40, 44, 50). For each action, replace the `@v<number>` tag with the full commit SHA (e.g., `@abc123def456...`) to ensure reproducibility and prevent supply chain drift from tag retargeting by maintainers. You can find the commit SHA for each action version by checking the action's GitHub release page or tag history.Source: Linters/SAST tools
src/alteryx_server_py/models/collections.py (2)
62-69: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add explicit typing and full function docstring on
model_dumpoverride (Line 62).Please type the override (
*args,**kwargs, return type) and include Google-styleArgs/Returnsin the docstring to keep this custom serialisation hook maintainable.Proposed patch
-from typing import Optional +from typing import Any, Optional @@ class _PermissionFlattenMixin: """Shared payload flattening for collection permission requests.""" - def model_dump(self, *args, **kwargs): - """Flatten nested permissions into the parent request payload.""" + def model_dump(self, *args: Any, **kwargs: Any) -> dict[str, Any]: + """Flatten nested permissions into the parent request payload. + + Args: + *args: Positional args forwarded to ``BaseModel.model_dump``. + **kwargs: Keyword args forwarded to ``BaseModel.model_dump``. + + Returns: + dict[str, Any]: Serialised payload with nested permissions flattened. + """ kwargs.setdefault("mode", "json") data = super().model_dump(*args, **kwargs) permissions = data.pop("permissions", None) if permissions: data.update(permissions) return dataAs per coding guidelines, "Follow PEP8, use type hints, and format with
black." and "Write docstrings for every function using Google style format with brief summary, Args, and Returns sections."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/alteryx_server_py/models/collections.py` around lines 62 - 69, The model_dump method override lacks explicit type hints and a complete Google-style docstring. Add type annotations to the method signature for *args, **kwargs, and the return type (the return type should match the parent class, typically Any or dict). Expand the docstring to include a brief summary (already present), followed by an Args section documenting the *args and **kwargs parameters, and a Returns section documenting what the method returns, following Google-style docstring format for maintainability.Source: Coding guidelines
110-110:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Locate imports/usages of legacy and split request types with nearby context. rg -nP -C3 '\bCollectionPermissionUpdateRequest\b|\bCollectionUserPermissionUpdateRequest\b|\bCollectionGroupPermissionUpdateRequest\b'Repository: Sivivatu/AlteryxGalleryAPI
Length of output: 7224
🏁 Script executed:
# Examine the full context of usages in collections.py resources file sed -n '280,295p' src/alteryx_server_py/resources/collections.py sed -n '545,560p' src/alteryx_server_py/resources/collections.pyRepository: Sivivatu/AlteryxGalleryAPI
Length of output: 1409
🏁 Script executed:
# Find the method signatures for the code that uses CollectionPermissionUpdateRequest # Look for methods around lines 270-295 and 540-560 sed -n '265,295p' src/alteryx_server_py/resources/collections.py sed -n '535,560p' src/alteryx_server_py/resources/collections.pyRepository: Sivivatu/AlteryxGalleryAPI
Length of output: 2447
🏁 Script executed:
# Get the exact model definitions for both request types sed -n '92,99p' src/alteryx_server_py/models/collections.py sed -n '101,108p' src/alteryx_server_py/models/collections.pyRepository: Sivivatu/AlteryxGalleryAPI
Length of output: 704
Remove or deprecate the legacy
CollectionPermissionUpdateRequestalias to prevent validation bypass in user permission updates.The alias at line 110 maps
CollectionPermissionUpdateRequesttoCollectionGroupPermissionUpdateRequest, makingexpiration_dateoptional. However,CollectionUserPermissionUpdateRequestrequires this field (line 92). Both update methods insrc/alteryx_server_py/resources/collections.pyuse the alias regardless of target (user vs. group), allowing user permission updates to skip the requiredexpiration_datevalidation. UseCollectionUserPermissionUpdateRequestexplicitly for user updates andCollectionGroupPermissionUpdateRequestfor group updates, or remove the alias entirely to enforce the correct field requirements by type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/alteryx_server_py/models/collections.py` at line 110, Remove the legacy `CollectionPermissionUpdateRequest` alias assignment that maps to `CollectionGroupPermissionUpdateRequest` in the collections.py models file. This alias causes validation bypass because `CollectionGroupPermissionUpdateRequest` has an optional `expiration_date` field while `CollectionUserPermissionUpdateRequest` requires it. Update the collection resource methods to explicitly use `CollectionUserPermissionUpdateRequest` for user permission updates and `CollectionGroupPermissionUpdateRequest` for group permission updates, ensuring that the correct field requirements are enforced based on the target update type.src/alteryx_server_py/models/workflows.py (1)
90-104: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add inline
# Reason:comments around legacy normalisation branches.Lines 99-104 implement non-trivial coercion/inference rules; please annotate why those defaults are chosen to make future maintenance safer.
Proposed patch
if workflow_type is None: + # Reason: Legacy payloads often omit workflowType; infer from legacy fields to preserve model compatibility. legacy_workflow_type = package_workflow_type or execution_mode normalized["workflowType"] = workflow_type_mapping.get(legacy_workflow_type, "Standard") if execution_mode not in valid_execution_modes: + # Reason: Unknown legacy execution modes must be coerced to a supported safe default. normalized["executionMode"] = "Safe"As per coding guidelines, "When writing complex logic, add an inline
# Reason:comment explaining the why, not just the what."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/alteryx_server_py/models/workflows.py` around lines 90 - 104, The legacy normalization branches in the workflow_type_mapping and execution_mode handling lack explanatory inline comments about why those specific defaults are chosen. Add inline # Reason: comments before the assignments to normalized["workflowType"] (where it uses workflow_type_mapping.get() with "Standard" as the fallback) and before the assignment to normalized["executionMode"] (where it defaults to "Safe"). These comments should explain the business rationale or reasoning for these specific default values to aid future maintenance.Source: Coding guidelines
tests/unit/models/test_credentials_model.py (1)
42-55:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd failure-path tests for required share identifiers (Line 42).
These tests only assert successful serialisation. Please add
ValidationErrorcases for missinguser_idanduser_group_idto protect required-field contracts.Proposed patch
import pytest +from pydantic import ValidationError @@ class TestCredentialModel: @@ def test_user_group_share_request_serialization(self): """Test credential user-group share request serialization.""" request = CredentialUserGroupShareRequest(user_group_id="group-1") data = request.model_dump(by_alias=True, exclude_none=True) assert data == {"userGroupId": "group-1"} + + def test_user_share_request_requires_user_id(self): + """Test credential user share request rejects missing user_id.""" + with pytest.raises(ValidationError): + CredentialUserShareRequest() + + def test_user_group_share_request_requires_user_group_id(self): + """Test credential user-group share request rejects missing user_group_id.""" + with pytest.raises(ValidationError): + CredentialUserGroupShareRequest()As per coding guidelines, "Always create Pytest unit tests for new features (functions, classes, routes, etc)." and "Include at least 1 test for expected use, 1 edge case, and 1 failure case in test coverage."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def test_user_share_request_serialization(self): """Test credential user share request serialization.""" request = CredentialUserShareRequest(user_id="user-1") data = request.model_dump(by_alias=True, exclude_none=True) assert data == {"userId": "user-1"} def test_user_group_share_request_serialization(self): """Test credential user-group share request serialization.""" request = CredentialUserGroupShareRequest(user_group_id="group-1") data = request.model_dump(by_alias=True, exclude_none=True) assert data == {"userGroupId": "group-1"} def test_user_share_request_requires_user_id(self): """Test credential user share request rejects missing user_id.""" with pytest.raises(ValidationError): CredentialUserShareRequest() def test_user_group_share_request_requires_user_group_id(self): """Test credential user-group share request rejects missing user_group_id.""" with pytest.raises(ValidationError): CredentialUserGroupShareRequest()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/models/test_credentials_model.py` around lines 42 - 55, The existing tests test_user_share_request_serialization and test_user_group_share_request_serialization only cover the happy path for successful serialization. Add two new failure-path tests: one that attempts to instantiate CredentialUserShareRequest without the required user_id parameter and expects a ValidationError to be raised, and another that attempts to instantiate CredentialUserGroupShareRequest without the required user_group_id parameter and expects a ValidationError to be raised. These tests will ensure the required-field contracts are properly validated and protect against missing identifier cases.Source: Coding guidelines
tests/unit/models/test_workflows_model.py (1)
64-76:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a failure-case test for invalid
executionModecoercion.The new Standard-mode tests are good, but this change set still lacks a failure case asserting that unsupported legacy values are normalised to
"Safe".Proposed patch
def test_workflow_preserves_standard_execution_mode(): @@ wf = Workflow.model_validate(payload) assert wf.execution_mode.value == "Standard" + + +def test_workflow_coerces_invalid_execution_mode_to_safe(): + payload = { + "id": "wf-invalid-mode", + "name": "Invalid Mode Workflow", + "ownerId": "sub-invalid", + "dateCreated": "2024-06-01T12:34:56Z", + "workflowType": "Standard", + "executionMode": "LegacyMode", + } + + wf = Workflow.model_validate(payload) + + assert wf.execution_mode.value == "Safe"As per coding guidelines, "Include at least 1 test for expected use, 1 edge case, and 1 failure case in test coverage."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/models/test_workflows_model.py` around lines 64 - 76, Add a new test function after test_workflow_preserves_standard_execution_mode that validates the failure case for invalid executionMode coercion. Create a payload with an unsupported or invalid executionMode value (e.g., a legacy or nonexistent value), pass it to Workflow.model_validate(), and assert that the resulting wf.execution_mode.value is normalized to "Safe" to ensure backward compatibility and safe defaults for unexpected values.Source: Coding guidelines
10f5c97 to
41abc4f
Compare
Dismissed stale CodeRabbit changes-requested review after addressing all actionable threads; latest CodeRabbit status is successful and no live unresolved non-outdated review threads remain.
|
@coderabbitai review |
✅ Action performedReview finished.
|
41abc4f to
a295dd4
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |

Summary by CodeRabbit
Release Notes
New Features
Improvements