Skip to content

Accept dots in the build id - #6

Merged
nick-potts merged 3 commits into
mainfrom
fix/deploys-32
Oct 19, 2026
Merged

nick-potts merged 3 commits into
mainfrom
fix/deploys-32

Conversation

@nick-potts

Copy link
Copy Markdown
Contributor

Every GitHub build Ployz Cloud dispatches fails in its first step with Invalid build id. Cloud names a build DEPLOYMENT.SERVICE (for example 36755d9a-dc30-4385-a9bd-86ae1a85e44b.ployz-soak-node), and prepare.sh only accepted [A-Za-z0-9_-]. Cloud then falls back to building on a Server, so the failure is quiet. Found in the beta.6 soak (deploys-32, run 39155660555).

The build id pattern now also accepts ., but not as the first character, so the id can never be the path segment . or .. in {cloud}/api/builds/{build}/.... The length cap stays at 128. Cloud's ids are at most 100 characters: a 36-character UUID, a dot, and a Service name of at most 63 characters.

Cloud keeps the dotted format because Machines already parse it (split_once('.') in ployz-store).

Testing

  • test.sh now runs with an id in Cloud's shape. It fails on main with ::error::Invalid build id. and passes here.
  • New cases check that ., .., ../x, a/b, a b and a 129-character id are still refused with Invalid build id.
  • shellcheck *.sh is clean.

After merge

Move the v1 tag to the merge commit (and tag v1.3.1). Workflows pin getployz/build@v1, so the fix reaches users only after v1 moves.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-18T23:30:43.873216Z 5f07a3e PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: fd617a9d-a04f-4d3c-b2b9-d6d9d75f79fd
📥 Commits

Reviewing files that changed from the base of the PR and between 5f07a3e and 087ff5f.

📒 Files selected for processing: 1
  • test.sh
💤 Files with no reviewable changes: 1
  • test.sh

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.

📝 Summary

Summary by CodeRabbit

  • Improvements
    • Build IDs may include periods after an initial letter, digit, underscore, or hyphen. The 128-character limit remains in place.
  • Validation
    • Build IDs containing path traversal, path separators, spaces, or more than 128 characters are rejected. IDs made up only of periods, including . and .., are also rejected.
  • Documentation
    • Updated the documented build input format to reflect support for periods and the existing length and starting-character restrictions.

Build ID validation and documentation now permit periods after the first character. The 128-character limit remains. Tests use a UUID-shaped build ID and check rejection of invalid values.

Changes: Build ID format

Layer / File(s) Summary
Build ID validation and coverage
prepare.sh, test.sh, README.md
Validation permits periods after the first character and retains the 128-character limit. Tests use a UUID-shaped ID and check rejection of traversal patterns, path separators, spaces, and IDs longer than 128 characters. The documented pattern includes periods and identifies the value as DEPLOYMENT.SERVICE.

Priority: ⬆️ High

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 087ff

Dotted build IDs are accepted in the tested build flow, while the specified invalid IDs remain rejected. No actionable merge risk remains.

Security architecture risk: 🔵 Low · up to 5f07a

The change preserves the length limit and single-path-segment restriction, and no introduced security defect was established. Authorization and recovery behavior for newly accepted dotted identifiers could not be verified on the Cloud side.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The security-relevant scope extends beyond identifier validation: newly admitted dotted values select check-in and reporting resources, whose responses supply deployment secrets and build authority toward a Cloud-selected Machine. Maximum cross-build, tenant, or Machine exposure cannot be bounded without server-side ownership and grant-enforcement evidence.

Security Findings and Attack Paths

  • inferred — No introduced attack path was established. Caller-supplied identifiers reach authenticated Cloud requests, but the new character set does not permit URL separators, query delimiters, percent encoding, or shell syntax. Whether an authenticated caller can select another build remains an unresolved authorization question, not an observed access-control failure.

Trust Boundaries and Controls

  • observed — The workflow supplies inputs through environment variables rather than inserting them into shell source. Preparation validates the identifier before check-in, and requests carry a GitHub OIDC token requested for the selected Cloud-origin audience. Those client-side controls do not demonstrate server-side binding between the run identity and requested build.

Resilience and Maintainability Implications

  • observed — Existing client behavior retries transport failures, stops on explicit HTTP refusal, and reports install failure and build completion. Documentation describes duplicate-batch handling, but server-side atomicity, concurrent ownership, terminal-state enforcement, and recovery after interruption remain unverified for newly accepted identifiers. The PR does not change the local recovery implementation.
Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description check Passed The description clearly explains why dotted build IDs are required, the validation rules, the tests, and the release step.
Title check Passed The title clearly and concisely describes the main change: allowing dots in build IDs.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@nick-potts
nick-potts merged commit 4efab52 into main Oct 19, 2026
4 checks passed
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.

1 participant