Skip to content

Validate plugin marketplace source URL scheme#608

Open
jlav wants to merge 1 commit intomainfrom
jl/plugin-marketplace-url-regex
Open

Validate plugin marketplace source URL scheme#608
jlav wants to merge 1 commit intomainfrom
jl/plugin-marketplace-url-regex

Conversation

@jlav
Copy link
Copy Markdown
Contributor

@jlav jlav commented May 5, 2026

Description

The plugin_directory_marketplace_source field on the Replicated config is free-form text. This adds a regex on the field so the Replicated console rejects invalid input at configuration time.

The accepted schemes (github://, https://, http://) match what the catalog loader supports. file:// is removed because it's only supported for local dev for now.

Helm Chart Checklist

  • I have updated the version field in Chart.yaml for each modified chart
  • I have tested the chart upgrade path from the previous version
  • I have verified backwards compatibility with existing values.yaml configurations
  • I have updated the chart's README.md if there are any breaking changes or new required values

Add a regex validation to plugin_directory_marketplace_source so the
Replicated console rejects URIs that won't resolve. Only github://,
https://, and http:// are supported by the catalog loader; file://
was previously mentioned in help text but isn't a valid runtime input.
Copy link
Copy Markdown
Contributor

@all-hands-bot all-hands-bot left a comment

Choose a reason for hiding this comment

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

🟡 Acceptable approach, but needs verification before merge

The validation pattern itself is clean and appropriate, but removing file:// support requires investigation of the actual catalog loader implementation to avoid breaking existing deployments.

Comment thread replicated/config.yaml
Comment thread replicated/config.yaml
@all-hands-bot
Copy link
Copy Markdown
Contributor

Code Review Analysis

Taste Rating

🟡 Acceptable - Clean validation pattern, but scope requires verification


[CRITICAL ISSUES]

[replicated/config.yaml, Line 678] Breaking Change - Unverified: Removing file:// from both help text and validation pattern creates a breaking change risk:

  • If the catalog loader (ghcr.io/openhands/plugin-directory-server) actually supports file:// URLs, this will break existing deployments
  • If any customers have file:// paths configured, their Replicated console will reject the config on upgrade
  • The PR description acknowledges uncertainty: "If file:// is in fact a supported runtime input, the regex needs to be widened"

Required before merge:

  1. Review the actual catalog loader source code to confirm which schemes are supported
  2. Search for existing customer deployments using file:// (support tickets, Slack, internal configs)
  3. If file:// is supported OR in use: expand regex to ^(github|https?|file):// and restore help text
  4. If file:// is confirmed unsupported: document the verification in the PR description

[TESTING GAPS]

[PR description] No Evidence: The PR description doesn't include proof that:

  • The validation works correctly (rejects invalid input with the expected error message)
  • Valid URLs (github://owner/repo, https://..., http://...) still pass
  • The Replicated console UI displays the validation error properly

Recommended testing (if accessible):

# Test valid inputs
github://openhands/plugins
https://raw.githubusercontent.com/openhands/plugins/main/catalog.json
http://internal-catalog-server/plugins.json

# Test invalid inputs (should be rejected)
gitub://typo/repo
file:///local/path/catalog.json
ftp://unsupported-scheme

[RISK ASSESSMENT]

Overall PR: ⚠️ Risk Assessment: 🟡 MEDIUM

Risk factors:

  • Breaking change potential: High if file:// is actually supported or in use
  • Limited blast radius: Only affects Replicated deployments using this specific config field
  • Easy rollback: Configuration change can be reverted quickly if issues arise
  • Validation is beneficial: Catching misconfigurations at config-time rather than runtime is the right approach

Mitigation:

  • Verify catalog loader implementation before merge (see Critical Issues above)
  • If uncertain about customer usage, consider a two-phase rollout:
    1. First PR: Add validation that includes file:// initially: ^(github|https?|file)://
    2. Second PR (after verification): Remove file:// if confirmed unsupported

VERDICT

Needs verification before merge: The validation pattern is sound, but the scope (which schemes to allow) must be verified against the actual catalog loader implementation to avoid breaking existing deployments.

Once verification is complete, this is a good improvement - catching config errors early is pragmatic and user-friendly.


KEY INSIGHT

Don't break userspace: verify the catalog loader implementation before removing a documented URL scheme, even if you suspect it was never actually supported.


Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

  1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
  2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
  3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

@jlav jlav requested a review from all-hands-bot May 6, 2026 12:01
Copy link
Copy Markdown
Contributor

@all-hands-bot all-hands-bot left a comment

Choose a reason for hiding this comment

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

🟢 Good taste

Simple validation that correctly matches the documented URL schemes. Previous concerns about validation scope and file:// removal have been addressed.

[RISK ASSESSMENT]

  • [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM

Client-side validation with low risk of breaking existing functionality. The worst case (false-positive rejection) would be immediately visible during configuration. Manual testing evidence would be valuable but is not blocking given the simplicity of the change.

VERDICT:
Worth merging

KEY INSIGHT:
Enforcing constraints at the configuration layer prevents invalid state from entering the system.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants