Skip to content

fix(deployments): preserve and normalize VCS source roots - #13574

Open
HarshMN2345 wants to merge 11 commits into
mainfrom
fix/root-directory-normalization
Open

fix(deployments): preserve and normalize VCS source roots#13574
HarshMN2345 wants to merge 11 commits into
mainfrom
fix/root-directory-normalization

Conversation

@HarshMN2345

@HarshMN2345 HarshMN2345 commented Sep 9, 2026

Copy link
Copy Markdown
Member

What does this PR do?

Preserve the source directory when building and redeploying VCS Functions and Sites. A webhook deployment that omits providerRootDirectory can 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.

  • Normalize artifact subdirectories and both template-worker paths through Deployments::rootDirectory(), preserving hidden directory names.
  • Save the source root in webhook deployment metadata and snapshot the actual root supplied to createFromVcs() for archive and clone builds.
  • When duplicating a connected deployment with a missing or null legacy root, use the Function/Site's current root and save it on the new deployment. Existing snapshots, including an explicit empty string for repository root, continue to take precedence over later resource settings. Public template duplication keeps its existing source selection.

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 ./docs archive 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.
  • Pint, targeted PHPStan, PHP syntax, and whitespace checks pass for the changed PHP files.
  • Gitea E2E regression covers nested-root webhook deployment, a seeded legacy null root, duplication after resource settings change, and an explicitly empty snapshot. It checks the returned execution's deployment ID and source-root environment variable.
  • The extended E2E was not run locally because the Appwrite/Gitea Docker stack is unavailable. Local unit tests disable the repository's Swoole-dependent test hook; the selected tests do not use coroutines. Site and public-template duplication are not covered by the added E2E.

Live check:

docker compose exec appwrite test tests/e2e/Services/VCSGitea/VCSGiteaConsoleClientTest.php --filter=testCreateDeploymentFromNestedRootDirectory

Related PRs and Issues

Checklist

  • Read the contributing guidelines.
  • API specs and example docs: not applicable; no API metadata changes.

… 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.
@greptile-apps

greptile-apps Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

The production changes appear sound, but the implementation-coupled unit test violates an explicit repository testing requirement and must be corrected before merging.

Fix All in Claude CodeFindings

  1. P2 Test Mirrors Internal Details
Fix with agent prompt
### Issue 1
tests/unit/Deployment/DeploymentsTest.php:54-58
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.

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!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

  • Canonicalizes archive and clone artifact subdirectories without corrupting dot-prefixed directory names.
  • Persists webhook and shared VCS deployment roots so later duplicates retain their source location.
  • Adds function and site compatibility fallback behavior for historical deployments.
  • Expands unit and Gitea end-to-end coverage for nested roots and duplication.
flowchart LR
  VCS[VCS deployment root] --> Persist[Persist raw providerRootDirectory]
  Persist --> Canonicalize[Canonicalize artifact subdirectory]
  Canonicalize --> Artifact[Clone or unarchive source]
  Persist --> Duplicate[Duplicate deployment]
  Legacy[Legacy deployment with null root] --> Fallback[Read current resource root]
  Fallback --> Duplicate
  Duplicate --> Persist
Loading

Reviews (9) · Last reviewed commit: "fix(deployments): preserve source roots ..."

Comment thread src/Appwrite/Deployment/Deployments.php Outdated
Comment thread src/Appwrite/Platform/Modules/VCS/Http/GitHub/Deployment.php
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.
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

✨ Benchmark results

Comparing main (before) → fix/root-directory-normalization (after).

Metric Before After Change
🚀 Requests/sec 177.35 199.16 🟢 +12.3%
⏱️ Latency P50 97.23 ms 86.88 ms 🟢 -10.6%
⏱️ Latency P95 231.99 ms 208.13 ms 🟢 -10.3%
Per-scenario breakdown & investigation details

Metrics below reflect the current branch (after). Δ P95 compares against the base.

Scenario P50 (ms) P95 (ms) Requests RPS Δ P95 (ms)
API total 86.88 208.13 12,540 199.16 -23.86
Account 171.4 323.54 660 11.04 -29.78
TablesDB 84.06 155.7 6,820 110.6 -19.24
Storage 79 172.56 3,300 55.65 -20.87
Functions 128.67 265.81 1,760 30.42 -10.66

Top API waits (after)

API request Max wait (ms)
functions.variables.update 542.38
account.name.update 508.41
account.prefs.update 482.05
tablesdb.rows.create 467.88
functions.delete 422.15

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.
Comment thread tests/unit/Deployment/DeploymentsTest.php Outdated
HarshMN2345 and others added 5 commits September 9, 2026 16:50
…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.
@HarshMN2345 HarshMN2345 changed the title fix(deployments): canonicalize the root directory an artifact indexes into fix(deployments): preserve and normalize VCS source roots Sep 14, 2026
Comment on lines +54 to +58
'commit',
$root,
);

$this->assertSame('waiting', $deployment->getAttribute('status'));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 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)

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!

Fix in Claude Code Fix in Codex

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.

🐛 Bug Report: Redeploy always fails for monorepo functions — duplicate reads providerRootDirectory from the deployment, where nothing ever sets it

1 participant