diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e69354c1..0ec869e4 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -46,6 +46,13 @@ years ago and still working without changes. Google App Engine engineers will review the proposed changes, and will decide if ultimately it can be pushed to production. If the change cannot be pushed for whatever reasons, the change will be rollbacked. +## Repository Layout Constraints + +This repository is synchronized with Google's internal source tree (`google3`) by Copybara. +Internal source code uses directories without leading dots, and Copybara rules will map +these to `.claude/`, `.agent/`, and `.gemini/` on GitHub (and back). In the interim before +that rule is active, agent skills are located in [agent_skills/](agent_skills/README.md). + ## Community Guidelines This project follows diff --git a/agent_skills/README.md b/agent_skills/README.md new file mode 100644 index 00000000..36e3ba29 --- /dev/null +++ b/agent_skills/README.md @@ -0,0 +1,62 @@ + + +# Agent Skills + +This directory holds skills for AI coding agents (Claude Code, Gemini CLI, and +others) working on this repository. Each subdirectory is one skill, with a +`SKILL.md` file that describes when to use it and the steps to follow. + +| Skill | Use it when | +| --- | --- | +| [internal-runtime-usage](internal-runtime-usage/SKILL.md) | Changing public API classes that the runtime itself calls, or their feature flags. | +| [pom-dependency-change](pom-dependency-change/SKILL.md) | Changing a Maven dependency, shade include, relocation or filter, or bumping a version. | +| [runtime-e2e-tests](runtime-e2e-tests/SKILL.md) | Building the runtime deployment, or running or writing tests in `runtime/test`. | +| [runtime-jar-deps](runtime-jar-deps/SKILL.md) | Changing API classes bundled into the runtime jars, or their shading. | + +## Repository layout and Copybara synchronization + +Google's internal source tree (`google3`) does not use leading dot directories for skills. +However, coding agents on GitHub (Claude Code, Antigravity, Gemini CLI) expect skills under +their respective dot directories (`.claude/skills/`, `.agent/skills/`, `.gemini/skills/`). + +To bridge this: +- **Phase 1 (Initial import)**: Skills are temporarily placed in this regular `agent_skills/` + directory without a leading dot so that Copybara can import the files into `google3` without + failing the import check. +- **Phase 2 (Bi-directional Copybara rule)**: A Copybara transformation rule in `google3` will + manage synchronization: + - **Export (`google3` -> GitHub)**: Rewrites the non-dot directories in `google3` to `.claude/`, + `.agent/`, `.gemini/` on GitHub so the skills appear natively in the right place. + - **Import (GitHub -> `google3`)**: Reverses the mapping, moving files from `.claude/`, `.agent/`, + `.gemini/` back to the corresponding non-dot directories in `google3`. + +Because these dot directories will be version-controlled, tracked files in Git on GitHub, +they **must not** be listed in `.gitignore`. + +### Local usage before the Copybara rule is deployed + +Until the bidirectional Copybara rule is in place in `google3`, you can point your local tools +at `agent_skills/` (for example, with a local symlink, without committing it): + +- **Claude Code**: + ```shell + mkdir -p .claude && ln -s ../agent_skills .claude/skills + ``` +- **Gemini CLI / Antigravity**: + ```shell + mkdir -p .agent && ln -s ../agent_skills .agent/skills + ``` diff --git a/agent_skills/internal-runtime-usage/SKILL.md b/agent_skills/internal-runtime-usage/SKILL.md new file mode 100644 index 00000000..1db489b3 --- /dev/null +++ b/agent_skills/internal-runtime-usage/SKILL.md @@ -0,0 +1,72 @@ +--- +name: internal-runtime-usage +description: Use when changing the behavior, dependencies or feature flags of public App Engine API classes (QueueImpl, QueueFactory, DatastoreServiceImpl, MemcacheServiceImpl, URLFetchService, UserService, ApiProxy, ...), or when adding an environment-variable switch to them such as APPENGINE_USE_CLOUDTASK_PUSH_QUEUE. Finds the places where the runtime itself calls these APIs from its private class loader and makes sure they are tested end to end in both modes. +--- + + + +# API classes used by the runtime itself + +Applications use the App Engine APIs through `appengine-api-1.0-sdk.jar`. The **runtime** also +uses some of them, from its own class loader, with the copies bundled in +`runtime-impl-jetty12.jar` / `runtime-impl-jetty121.jar`. A change that works for applications can +therefore still break the runtime, and unit tests in `api/` do not cover it. + +## 1. Find runtime callers + +Search the code that runs in the runtime class loader: + +``` +grep -rln --include=*.java -e 'QueueFactory\|QueueImpl\|DatastoreServiceFactory\|MemcacheServiceFactory\|URLFetchServiceFactory\|UserServiceFactory\|ApiProxy' \ + shared_sdk shared_sdk_jetty12 shared_sdk_jetty121 runtime/impl runtime/runtime_impl_jetty12 runtime/runtime_impl_jetty121 runtime/lite runtime/util api/src/main/java/com/google/apphosting/utils/servlet +``` + +Narrow the pattern to the class you changed. Known runtime users include: + +| Runtime code | API used | Enabled by | +|---|---|---| +| `DeferredDatastoreSessionStore` (shared_sdk_jetty12/121) | Task Queue (`QueueFactory`, `DeferredTask`) | `` + `` | +| `DatastoreSessionStore`, `MemcacheSessionStore` | Datastore, Memcache | `true` | +| `DeferredTaskServlet` (bundled in the runtime jars) | runs `DeferredTask` payloads (Task Queue) | deferred tasks, including async session writes | +| `SessionCleanupServlet` (bundled in the runtime jars) | Datastore | expired session cleanup | +| `SweeperServlet` (bundled in the runtime jars) | `TaskProcessor` → Cloud Tasks | `APPENGINE_USE_CLOUDTASK_PUSH_QUEUE` | +| `AppEngineLoginService`, `AppEngineAuthentication`, `AppEngineUserIdentity` | Users API | `` in `web.xml` | + +Also check which API packages are bundled: the `` for `com.google.appengine:appengine-apis` +in `runtime/runtime_impl_jetty12/pom.xml` and `runtime/runtime_impl_jetty121/pom.xml`. Any class in +those packages, including new ones you add, ends up in the runtime jars. + +## 2. Test the runtime path end to end + +Unit tests are not enough: add or extend a test in `runtime/test` that starts the real runtime +(`JavaRuntimeViaHttpBase`) with an app that triggers the runtime caller. If the change is behind a +flag, run the test **with the flag on** (and keep the existing test for the flag off). +`CloudTasksSessionTest` is the template: it copies the allinone app, enables +`` in its `appengine-web.xml`, sets +`APPENGINE_USE_CLOUDTASK_PUSH_QUEUE=true` through `setEnvironmentEntries`, and asserts that the +requests succeed and that `outputLinesContaining(...)` finds no `ClassNotFoundException` / +`NoClassDefFoundError`. See the `runtime-e2e-tests` skill to build and run it. + +## 3. Check the bundled dependencies + +If the change makes the API classes use a new library, follow the `runtime-jar-deps` skill. + +## 4. Say what the internal build needs + +The internal (google3) build uses BUILD files, not these poms. In the PR, list the libraries the +runtime jars now need so the matching BUILD deps can be added internally, otherwise the change +can pass here and fail internally. diff --git a/agent_skills/pom-dependency-change/SKILL.md b/agent_skills/pom-dependency-change/SKILL.md new file mode 100644 index 00000000..6c849bdd --- /dev/null +++ b/agent_skills/pom-dependency-change/SKILL.md @@ -0,0 +1,84 @@ +--- +name: pom-dependency-change +description: Use before removing or adding a Maven dependency, an artifactSet include, a relocation or a filter in any pom.xml of this repo, or when bumping a library version (including reviewing Dependabot PRs). Checks why the entry exists, what depends on it transitively, and how the shaded jars (appengine-api-1.0-sdk, remoteapi, runtime_impl) change, including their size. +--- + + + +# Changing Maven dependencies safely + +Many pom entries look unused but exist for a shaded jar or a runtime-only code path. For example +`io.grpc:grpc-api` in `appengine-api-1.0-sdk/pom.xml` was added for issue #381 (shaded OpenCensus +in remote_api needs `io.grpc.Context`); removing it later only kept working because another +library happened to pull it in. + +## 1. Find why the entry exists + +``` +git log --format='%h %ad %an %s' --date=short -S'ARTIFACT' -- '*pom.xml' +git log --format='%h %ad %s' --date=short -G'ARTIFACT' -- '*pom.xml' +``` + +Read the commit that added it (`git show `), and any XML comment next to the entry. Search the +issue tracker for referenced issue numbers. + +## 2. Compare the dependency trees + +Run before and after the change, for every module whose pom you touch and for the shaded modules +that depend on it: + +``` +./mvnw -q dependency:tree -pl MODULE -Dincludes=GROUP_ID -DoutputFile=/tmp/tree-before.txt +``` + +The tree omits duplicates, so a library that disappears from one branch may still be resolved +elsewhere; check the final resolved version, not only the node you edited. + +## 3. Check the shaded jars + +| Jar | Pom | What to check | +|---|---|---| +| `appengine-api-1.0-sdk.jar` (used by apps) | `appengine-api-1.0-sdk/pom.xml` | `` includes and `com.google.appengine.repackaged.*` relocations | +| `appengine-remote-api.jar` | `remoteapi/pom.xml` | `./mvnw verify -pl remoteapi` (runs `ShadingIT`) | +| `runtime-impl-jetty12.jar`, `runtime-impl-jetty121.jar` | `runtime/runtime_impl_jetty12*/pom.xml` | the `runtime-jar-deps` skill (`RuntimeJarDependencyClosureTest`) | + +Shade only bundles artifacts listed in an `artifactSet`; a library that is resolved but not listed +is silently left out of the jar. + +## 4. Report the size impact + +Build the jar before and after (use a `git worktree` of `origin/main` for "before", with +`./mvnw package` so `~/.m2` is not overwritten), then: + +``` +agent_skills/pom-dependency-change/scripts/jar_size_diff.sh OLD.jar NEW.jar +``` + +Put the totals and the largest packages in the PR description or a PR comment. + +## 5. Version pins with constraints + +Some versions are deliberately held back and guarded, for example `okio.version` must stay 1.x +(okio 2+ needs kotlin-stdlib, which the runtime jars do not bundle): it is enforced by a +`requireProperty` rule in the runtime_impl poms and ignored for `>= 2.0.0` in +`.github/dependabot.yml`. When you add such a pin, add the same guard and the Dependabot ignore, +and do not "fix" a failing guard by loosening it. + +## 6. Internal build + +The internal (google3) build uses BUILD files, not these poms. When a shaded jar gains or loses a +library, say so in the PR so the internal BUILD deps can be updated. diff --git a/agent_skills/pom-dependency-change/scripts/jar_size_diff.sh b/agent_skills/pom-dependency-change/scripts/jar_size_diff.sh new file mode 100755 index 00000000..f9d5f0e2 --- /dev/null +++ b/agent_skills/pom-dependency-change/scripts/jar_size_diff.sh @@ -0,0 +1,63 @@ +#!/usr/bin/env bash +# Copyright 2026 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Compares two builds of the same jar: total size, entry and class counts, and the compressed +# bytes of added and removed entries grouped by package (first three path segments). +# +# Usage: jar_size_diff.sh OLD.jar NEW.jar +set -euo pipefail +export LC_ALL=C + +if [[ $# -ne 2 ]]; then + echo "Usage: $0 OLD.jar NEW.jar" >&2 + exit 1 +fi +old=$1 +new=$2 +tmp=$(mktemp -d) +trap 'rm -rf "$tmp"' EXIT + +# Writes "pathcompressed-size" for each file entry of a jar, sorted by path. +entries() { + zipinfo -l "$1" | awk 'NF >= 10 && $1 ~ /^-/ { print $NF "\t" $6 }' | sort +} +entries "$old" > "$tmp/old" +entries "$new" > "$tmp/new" + +size_old=$(stat -c%s "$old") +size_new=$(stat -c%s "$new") +classes_old=$(grep -c '\.class ' "$tmp/old" || true) +classes_new=$(grep -c '\.class ' "$tmp/new" || true) +printf '%-10s %12s %12s %12s\n' "" "old" "new" "delta" +printf '%-10s %12d %12d %+12d\n' "bytes" "$size_old" "$size_new" $((size_new - size_old)) +printf '%-10s %12d %12d %+12d\n' "entries" "$(wc -l < "$tmp/old")" "$(wc -l < "$tmp/new")" \ + $(($(wc -l < "$tmp/new") - $(wc -l < "$tmp/old"))) +printf '%-10s %12d %12d %+12d\n' "classes" "$classes_old" "$classes_new" \ + $((classes_new - classes_old)) + +# Prints compressed bytes and entry counts per package for entries only in the first file. +by_package() { + join -t "$(printf '\t')" -v 1 "$1" <(cut -f1 "$2") | + awk -F'\t' '{ n = split($1, p, "/"); k = p[1]; for (i = 2; i <= 3 && i < n; i++) k = k "/" p[i]; + bytes[k] += $2; count[k]++ } + END { for (k in bytes) printf "%10d B %6d entries %s\n", bytes[k], count[k], k }' | + sort -rn +} +echo +echo "Added (compressed bytes by package):" +by_package "$tmp/new" "$tmp/old" +echo +echo "Removed (compressed bytes by package):" +by_package "$tmp/old" "$tmp/new" diff --git a/agent_skills/runtime-e2e-tests/SKILL.md b/agent_skills/runtime-e2e-tests/SKILL.md new file mode 100644 index 00000000..eae12a17 --- /dev/null +++ b/agent_skills/runtime-e2e-tests/SKILL.md @@ -0,0 +1,75 @@ +--- +name: runtime-e2e-tests +description: Use when building the runtime deployment or running or writing end-to-end tests in runtime/test (JavaRuntimeViaHttpBase, JavaRuntimeAllInOneTest, CloudTasksSessionTest, RuntimeJarDependencyClosureTest, ...). Covers the build commands, JDK requirements, how test parameters depend on the JDK, and common local failures that are not real regressions. +--- + + + +# Runtime end-to-end tests (runtime/test) + +Tests in `runtime/test` start the real runtime as a separate process from +`runtime/deployment/target/runtime-deployment-/` (the `appengine.runtime.dir` system +property, set by the root pom) and talk to it over HTTP, with a fake API server +(`HttpApiServer` / `DummyApiServer`). + +## Build + +- A full JDK is required (`javac` and `lib/ct.sym`); a JRE fails with + `release version 17 not supported`. Set `JAVA_HOME` to the JDK. +- Build what the tests need, skipping tests and integration tests: + + ``` + ./mvnw install -DskipTests -DskipITs -pl runtime/deployment,runtime/test -am + ``` + + `-pl runtime/test -am` alone does **not** rebuild the runtime jars or the deployment, so the + tests would run an old runtime. After changing a runtime_impl pom, rebuild with + `-pl runtime/runtime_impl_jetty12,runtime/runtime_impl_jetty121,runtime/deployment`. +- `-DskipTests` does not skip failsafe ITs (e.g. `remoteapi` `ShadingIT`); add `-DskipITs`. +- Do not resume a parallel (`-T`) build with `-rf`: modules that were not built yet are skipped. + +## Run + +``` +./mvnw test -pl runtime/test -Dtest=CloudTasksSessionTest -Dsurefire.failIfNoSpecifiedTests=false +``` + +`JavaRuntimeViaHttpBase.allVersions()` only returns the runtime/Jetty/EE combinations supported by +the JDK running the tests: on JDK 25 only `java25` (Jetty 12.1, EE8/EE11), which uses +`runtime-impl-jetty121.jar`. `runtime-impl-jetty12.jar` (Jetty 12.0) is only exercised on JDK 17 +and 21 (CI runs 17, 21, 25 and 28-ea). On JDK 24+ the build adds JVM flags that JDK 21 rejects, so +`-Djvm=/bin/java` does not work; rely on CI or build with JDK 21. + +## Local failures that are not regressions + +- `FailureFilterTest`, `AnnotationScanningTest`, `NoGaeApisTest`, `AsyncServletAppTest`: + `expected to be true` on `appRoot.isDirectory()` means their test web app (e.g. + `runtime/failinitfilterwebapp/target/failinitfilterwebapp-`) was not built for the + current version. Build those modules too, or run the full build. +- `GuestBookTest` runs a nested `mvnw clean install` and needs `sdk_assembly`. +- Cloud Tasks: there is no endpoint override, so with `APPENGINE_USE_CLOUDTASK_PUSH_QUEUE=true` the + client calls the real `cloudtasks.googleapis.com` and gets an auth error (`UNAUTHENTICATED` in + CI, `PERMISSION_DENIED` with local credentials). Tests must not depend on the task being + delivered. + +## Writing a test + +Extend `JavaRuntimeViaHttpBase`, copy an app with `copyAppToDir(...)`, patch its +`WEB-INF/appengine-web.xml` if needed, pass environment variables with +`RuntimeContext.Config.builder(...).setEnvironmentEntries(...)`, and check the runtime's +stdout/stderr with `RuntimeContext.outputLinesContaining(Pattern)` or +`awaitStdoutLineMatching`/`awaitStderrLineMatching`. `CloudTasksSessionTest` is a short example. diff --git a/agent_skills/runtime-jar-deps/SKILL.md b/agent_skills/runtime-jar-deps/SKILL.md new file mode 100644 index 00000000..096d514a --- /dev/null +++ b/agent_skills/runtime-jar-deps/SKILL.md @@ -0,0 +1,83 @@ +--- +name: runtime-jar-deps +description: Use when changing App Engine API classes that are bundled into the runtime jars (com.google.appengine.api.* such as taskqueue, datastore, memcache, urlfetch, users, appidentity, oauth; com.google.apphosting.utils.servlet.*), when adding a library dependency to those API classes, or when editing the shade configuration of runtime/runtime_impl_jetty12 or runtime/runtime_impl_jetty121. Verifies that every class the runtime can reach is actually bundled, so the runtime does not fail with NoClassDefFoundError. +--- + + + +# Runtime jar dependency closure + +The runtime jars `runtime-impl-jetty12.jar` and `runtime-impl-jetty121.jar` are built by the +maven-shade-plugin in `runtime/runtime_impl_jetty12/pom.xml` and +`runtime/runtime_impl_jetty121/pom.xml`. They copy selected App Engine API classes out of +`appengine-apis` (see the `` for `com.google.appengine:appengine-apis`, e.g. +`com/google/appengine/api/taskqueue/*`) and bundle third-party libraries from an explicit +allowlist (the ``). + +The runtime uses those API classes **from its own private class loader**, not the application's +(for example `DeferredDatastoreSessionStore` adds Task Queue tasks for async session +persistence). Nothing relocates or bundles a library automatically: if an API class starts using a +new library and the library is not in the `artifactSet`, the runtime fails only at run time: + +``` +NoClassDefFoundError: com/google/auth/Credentials + at com.google.appengine.api.taskqueue.QueueImpl.isCloudTaskBackendEnabled(...) + at com.google.apphosting.runtime.jetty.DeferredDatastoreSessionStore.doStore(...) +``` + +`appengine-api-1.0-sdk.jar` (what applications use) is a different shade with its own +`artifactSet` and relocations; fixing one does not fix the other. + +## Check + +1. Build the runtime deployment (needs a full JDK with `javac`; see the `runtime-e2e-tests` + skill for build details): + + ``` + ./mvnw install -DskipTests -DskipITs -pl runtime/deployment,runtime/test -am + ``` + +2. Run the closure test: + + ``` + ./mvnw test -pl runtime/test -Dtest=RuntimeJarDependencyClosureTest -Dsurefire.failIfNoSpecifiedTests=false + ``` + + `runtime/test/src/test/java/com/google/apphosting/runtime/RuntimeJarDependencyClosureTest.java` + runs `jdeps` on each runtime-impl jar with the other deployment jars on the class path, follows + every reference from `com.google.appengine.*` / `com.google.apphosting.*` classes through the + bundled libraries, and lists each missing class with the chain that reaches it, e.g. + `com.google.auth.Credentials <- com.google.appengine.api.taskqueue.CloudTasksClientWrapper`. + +## Fix a failure + +- **The code path is used by the runtime** (default assumption): add the artifact that contains + the missing class to the `` of **both** runtime_impl poms. Find the + artifact with `./mvnw dependency:tree -pl runtime/runtime_impl_jetty121 -Dincludes=` + or by looking at the same library in `appengine-api-1.0-sdk/pom.xml`. Repeat until the test + passes; libraries often need further libraries (e.g. `CloudTasksClient` needs `gax-httpjson` + even for gRPC). +- Libraries that register services in `META-INF/services` (gRPC providers, etc.) rely on the + `ServicesResourceTransformer` in the shade configuration; keep it. +- **The code path is never used by the runtime**: add the package to `KNOWN_MISSING_PACKAGES` in + the test, with the reason. Do not use this to hide a library the runtime needs. +- Check the size impact with `agent_skills/pom-dependency-change/scripts/jar_size_diff.sh` and + report it in the PR. +- The internal (google3) build does not use these poms. Say in the PR that the same libraries must + be added to the internal BUILD deps of the runtime jars. +- Add or extend an end-to-end test for the runtime code path (see the `internal-runtime-usage` + skill).