fix(deployments): preserve and normalize VCS source roots - #13574
fix(deployments): preserve and normalize VCS source roots#13574HarshMN2345 wants to merge 11 commits into
Conversation
… into `providerRootDirectory` reached the jobs-service through `trim($subdir, '/')`, which strips slashes but not dots. `docs`, `docs/` and `/docs` all canonicalized to `docs`, while `./docs` survived verbatim as an artifact subdir naming a path segment that does not exist in the extracted tree. `.` and `./` were worse: they collapsed to a non-empty `.`, so the repository-root case emitted a subdir instead of omitting one. Canonicalize on the consume side, in `Deployments::rootDirectory()`. Every remote-source build funnels through `submit()`, so one choke point covers new writes, rows already holding `./docs`, webhooks, duplicates and templates without a migration. Drop whole `.` segments rather than trimming the character. The builds worker's `ltrim($path, '.')` turned `.github` into `github` — a silent wrong-directory build — and left `../etc` traversal intact. Both worker call sites now share the canonicalizer, and the jobs-service hand-off passes the canonicalized value instead of re-reading the raw attribute, which also closes a mismatch where the template push committed at a space-stripped path but sent the space-bearing one. `..` segments are dropped rather than resolved, so a subdir cannot escape the tree it indexes into.
The shared deployment document carried every other `provider*` field but not `providerRootDirectory`, so a push-triggered deployment persisted an empty root directory. The build itself was correct — the root directory is passed separately — but duplicating such a deployment reads the value back off the source document, so a redeploy silently built from the repository root. The trait is shared by GitHub, GitLab, Gitea, Bitbucket and Origin.
|
Dropping `..` segments made `docs/../x` build `docs/x`, which is neither the directory the caller named nor the one POSIX would resolve — and if both exist, the wrong application deploys with no warning. Resolving the segment instead would let the subdir climb out of the tree it indexes into. Refuse the path instead. The failure is the owner's to fix, so it joins the refused-variable-key case in carrying its reason into the build log rather than a generic internal error.
✨ Benchmark resultsComparing
Per-scenario breakdown & investigation detailsMetrics below reflect the current branch (after). Δ P95 compares against the base.
Top API waits (after)
|
Builds a function from './docs/nested/' — the form the console's directory picker writes — over the Gitea suite, the only live-VCS suite CI boots. Covers both halves observably: the push-created deployment must execute the nested entrypoint, and duplicating it must reach 'ready'. A duplicate reads the root directory back off the source deployment, so a regression that stops persisting it builds from the repository root, where the auto-init README is the only file and no entrypoint exists.
…uild The deployment response model does not expose providerRootDirectory, so reading it off the body asserted on a key that is never returned. Duplicating the push-created deployment proves the same thing through behavior: the duplicate reads the root directory back off the deployment it copies, so it only reaches 'ready' if the push persisted one. Also deploy explicitly before pushing, so the nested entrypoint is observed executing on a deployment the test activated rather than one it assumed was active.
Refusing `..` added a failure mode where there was none: `trim($subdir, '/')` has always passed a parent segment through untouched, so `docs/../x` reaches the extractor today and resolves to the directory it names. Rejecting it before job submission turns a working deployment into a 400. Canonicalize the forms that actually diverge — empty and `.` segments — and leave the rest of the path as given. Dropping `..` instead would silently build `docs/x`, which is the reason it was singled out in the first place.
| 'commit', | ||
| $root, | ||
| ); | ||
|
|
||
| $this->assertSame('waiting', $deployment->getAttribute('status')); |
There was a problem hiding this comment.
This test repeats the supplied root across database state, APPWRITE_VCS_ROOT_DIRECTORY, and artifact serialization instead of observing which application is built. It can remain green while deployment source selection is wrong, and harmless storage or payload refactors can break it. The repository requires tests of observable behavior rather than tests that mirror source code or configuration, so this requirement must be satisfied before merging. Retain the end-to-end execution coverage and replace these assertions with behavior visible at the deployment boundary.
Context Used: Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/unit/Deployment/DeploymentsTest.php
Line: 54-58
Comment:
**Test Mirrors Internal Details**
This test repeats the supplied root across database state, `APPWRITE_VCS_ROOT_DIRECTORY`, and artifact serialization instead of observing which application is built. It can remain green while deployment source selection is wrong, and harmless storage or payload refactors can break it. The repository requires tests of observable behavior rather than tests that mirror source code or configuration, so this requirement must be satisfied before merging. Retain the end-to-end execution coverage and replace these assertions with behavior visible at the deployment boundary.
**Context Used:** Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
What does this PR do?
Preserve the source directory when building and redeploying VCS Functions and Sites. A webhook deployment that omits
providerRootDirectorycan build successfully but later redeploy from the repository root, where a monorepo's entrypoint and package files are absent. The build worker also strips the leading dot from hidden directories such as.github.Deployments::rootDirectory(), preserving hidden directory names.createFromVcs()for archive and clone builds.Legacy metadata cannot recover a historical root after the resource setting has already changed; its fallback uses the current setting. Artifact normalization leaves parent segments for the orchestrator to resolve. The orchestrator already handles
./docsarchive offsets, so this PR does not claim those archive builds were failing.Test Plan
php vendor/bin/phpunit --no-extensions tests/unit/Deployment: 39 tests, 60 assertions pass, using this branch's locked dependencies. Covers normalization, artifact payloads, and source-root persistence/environment/subdirectory behavior across archive and clone builds. All four new persistence cases fail when the snapshot write is removed.Live check:
Related PRs and Issues
Checklist