メニュー
Provides rules, patterns, and best practices for Gerrit backend system logic, Java APIs, performance, and correctness.
デフォルトでは、最初にソースを確認する Prompt が選択されています。直接コマンドに切り替えるか、ローカルコピーをダウンロードすることもできます。
インストールを決める前に、SKILL.md と SkillsMP に表示されている付属ファイルをお読みください。
Codex または Claude でインストール この Prompt をコピーして Codex、Claude、または他のアシスタントに貼り付けると、Skill ページを確認してインストールできます。
直接コマンドでは確認用 Prompt が省略されます。実行前にソースを確認してください。
npx skills add https://github.com/GerritCodeReview/gerrit --skill gerrit-system-logicコマンドは1行のまま表示されます。コピー前に横へスクロールして全体を確認してください。
ローカルで確認しますか？SkillsMP が現在取得できるファイルをダウンロードできます。
SKILL.md を表示中
Provides rules, patterns, and best practices for code hygiene, formatting, downstream plugin dependencies, and release operations in Gerrit.
Reviews TypeScript code against the Google TS Style Guide.
Proofreads and suggests structural improvements for Git commit messages to ensure style guide compliance, completeness, and accuracy.
SOC 職業分類に基づく
| name | gerrit-system-logic |
| description | Provides rules, patterns, and best practices for Gerrit backend system logic, Java APIs, performance, and correctness. |
This engineering guide serves as the definitive reference for maintaining system logic and operational correctness within Gerrit's repository management and code review infrastructure. It exists to capture critical historical tribal knowledge, prevent the regression of known failure modes—such as JGit thread-safety violations or NoteDb schema lock-ins—and establish strict, non-negotiable architectural boundaries for newly contributed code. By standardizing these patterns, the guide provides incoming engineers with the necessary context to safely navigate and modify highly concurrent, state-dependent subsystems.
Historically, undocumented assumptions surrounding asynchronous task resolution, object cache sizes, and concurrent worker pools led to silent data omissions, degraded latency, and complex race conditions. To mitigate this, the guide enforces strict constraints around execution environments, including explicit per-thread JGit object isolation, decoupled persistence models, and two-step schema rollouts. It additionally hardens the system's security posture by formalizing impersonation tracking, standardizing Contributor License Agreement (CLA) checks, and preventing identity-cycling vulnerabilities through rigorous account validation protocols.
The overarching technical domains covered within this repository span concurrent thread safety, resilient NoteDb serialization and Protobuf schema evolution, optimized REST API payload handling, deterministic Guice-based test sandboxing, and Bazel build infrastructure alignment. Adhering to these documented policies guarantees that the platform remains scalable, secure, and resilient against regressions across both internal infrastructure workflows and distributed cluster upgrades.
| Chapter Theme / Title | Scope & Objective |
|---|---|
| JGit Concurrency & Thread Safety | To prevent severe race conditions and |
| : : data corruption during parallel : | |
| : : processing, Gerrit requires strict : | |
| : : per-thread isolation of JGit : | |
: : resources like RevWalk, : | |
: : Repository, and ObjectReader. : | |
| : : Never share these non-thread-safe : | |
: : instances across ExecutorService : | |
| : : boundaries or parallel streams. : | |
| **NoteDb Serialization & Schema | Governs the serialization and schema |
| : Evolution** : evolution strategies for persisting : | |
| : : Gerrit change metadata to Git notes : | |
| : : (NoteDb). Strictly enforces two-step : | |
| : : schema rollouts, decoupled transfer : | |
| : : objects, and permissive JSON parsing : | |
| : : to guarantee data integrity across : | |
| : : distributed cluster upgrades. : | |
| **Query Engine Performance & | Governs the configuration, |
| : Operator Enforcement** : evaluation, and strict syntactic : | |
| : : enforcement of query predicates : | |
| : : within the repository engine. Focuses : | |
| : : on decoupling strict user-facing : | |
| : : evaluation constraints from : | |
| : : background observability and : | |
| : : implementing phased, non-blocking : | |
| : : rollouts to prevent configuration : | |
| : : deadlocks. : | |
| **Access Control & Impersonation | Governs the evaluation and auditing |
| : Security** : of impersonated access operations : | |
: : (e.g., SUBMIT_AS and RUN_AS). : | |
| : : Defines strict mechanisms for : | |
| : : isolating the initiating "real user" : | |
| : : from the "impersonated user" during : | |
| : : dynamic permission checks, and : | |
| : : mandates automated, centralized : | |
| : : logging in NoteDb and Protobuf to : | |
| : : guarantee uncorrupted audit trails. : | |
| **Account Lifecycle & Vulnerability | Defines constraints for handling |
| : Mitigation** : deleted or recreated user accounts to : | |
| : : prevent identity-cycling exploits, : | |
| : : specifically isolating validation : | |
| : : checks and filtering orphaned : | |
| : : approvals during merge operations : | |
| : : without degrading system performance. : | |
| **REST API Resource Routing & | REST API endpoints must strictly |
| : Payload Optimization** : reflect hierarchical entity : | |
| : : relationships while optimizing JSON : | |
| : : payloads to minimize bandwidth and : | |
| : : processing latency. Modifications to : | |
| : : serialization pipelines must : | |
| : : explicitly gate expensive permission : | |
| : : checks and gracefully handle : | |
| : : redundant or experimental fields : | |
| : : without causing UI ambiguity. : | |
| **Build Infrastructure & Dependency | Governs the configuration, |
| : Alignment** : versioning, and migration of the : | |
| : : build system infrastructure, focusing : | |
: : heavily on Bazel modules (bzlmod) : | |
| : : and dependency graphs. Establishes : | |
| : : constraints to ensure strict version : | |
| : : alignment, prevent compliance : | |
| : : pipeline stalls, and maintain : | |
| : : reliable developer bootstrapping : | |
| : : environments. : | |
| **Asynchronous Notification | Asynchronous notification pipelines |
| : Consistency** : must capture exact entity state at : | |
| : : the moment of initialization rather : | |
| : : than fetching it dynamically during : | |
| : : execution. This guarantees data : | |
| : : consistency and prevents delayed : | |
| : : background tasks from inadvertently : | |
| : : processing future, out-of-band : | |
| : : updates from persistent storage. : | |
| **Test Suite Configuration & | Dictates the implementation of |
| : Isolation** : parameterized integration tests, : | |
| : : strict Guice dependency injection, : | |
| : : and granular flaky test isolation. : | |
| : : Ensures deterministic test coverage : | |
| : : across multiple backend : | |
| : : configurations without leaking : | |
| : : generic APIs or brittle global : | |
| : : states. : | |
| Diff Engine Computation Limits | Explicitly tracking timeouts and |
| : : fallback states for expensive file : | |
| : : diff computations guarantees that : | |
| : : frontend clients are alerted to : | |
| : : incomplete operations. This prevents : | |
| : : silent data omission and ensures : | |
| : : accurate representation of modified : | |
| : : files in the user interface. : | |
| Concurrent Formatting & Caching | Parallelizing expensive API |
| : : formatting requires strict adherence : | |
| : : to explicit thread-local context : | |
| : : propagation, cache isolation for : | |
| : : mutated elements, and collection : | |
| : : pre-allocation. Failing to manage : | |
| : : concurrency strictly leads to context : | |
| : : leakage across parallel streams, : | |
| : : memory overhead, and poisoned UI : | |
| : : states. : | |
| **Protobuf Schema Management & | Governs the mapping between Java |
| : Conversion** : entities and Protobuf messages, : | |
| : : mandating automated reflection-based : | |
| : : validation and standardized converter : | |
| : : abstractions to prevent cache : | |
| : : inconsistencies and silent field : | |
| : : loss. : | |
| Compliance & CLA Enforcement | Strictly enforces Contributor License |
| : : Agreement (CLA) checks across all : | |
| : : administrative REST API endpoints : | |
| : : that modify project configurations : | |
| : : and access rules. This guarantees : | |
| : : compliance consistency across both : | |
| : : direct code pushes and API-driven : | |
| : : administrative actions. : |
Context: To prevent severe race conditions and data corruption during
parallel processing, Gerrit requires strict per-thread isolation of JGit
resources like RevWalk, Repository, and ObjectReader. Never share these
non-thread-safe instances across ExecutorService boundaries or parallel
streams.
| Rule ID | Principle / Constraint | Priority | Primary Symptom / Trap |
|---|---|---|---|
| T1-01 | Strict Thread Isolation | Critical | Passing a shared |
: : of JGit RevWalk Instances : : RevWalk instance from : | |||
| : : : : the main thread into : | |||
| : : : : worker tasks submitted to : | |||
| : : : : an executor. : | |||
| T1-02 | Thread-Safe JGit | Critical | Passing a single shared |
| : : Repository Instantiation : : JGit Repository instance : | |||
| : : for Concurrent : : to multiple asynchronous : | |||
| : : Computation : : worker threads. : |
Rule: Always instantiate JGit objects like
RevWalk,Repository, andObjectReaderper-thread when parallelizing tasks. Never share a single instance across worker threads in anExecutorService.What: JGit objects like
RevWalk,Repository, andObjectReaderare not thread-safe and must be strictly instantiated per-thread when parallelizing tasks via anExecutorService.Applies To: JGit operations, specifically diff computation and object parsing within multi-threaded ExecutorServices.
Why: Sharing a single RevWalk instance across parallel diff evaluation tasks led to severe race conditions, resulting in corrupted diff results, application crashes, or thread deadlocks. Failing to adhere to this typically results in Race Conditions / Data Corruption.
Trap 1: Passing a shared RevWalk instance from the main thread into worker
tasks submitted to an executor.
Don't:
// BAD: Sharing a single RevWalk across executor threads
RevWalk sharedRw = new RevWalk(repo);
for (Key key : keys) {
executor.submit(() -> evaluator.execute(key, sharedRw));
}
Do:
// GOOD: Opening a new Repository and RevWalk inside the submitted task
for (Key key : keys) {
executor.submit(() -> {
try (Repository threadRepo = repoManager.openRepository(project);
ObjectReader threadReader = threadRepo.newObjectReader();
RevWalk threadRw = new RevWalk(threadReader)) {
return evaluator.execute(key, threadRw);
}
});
}
Rule: Must open individual
Repositoryinstances for each worker thread when parallelizing background computations.What: JGit's
openRepositoryreturns aRepositoryobject that is not thread-safe. When parallelizing computations (such as file diffing), each thread or worker must instantiate its own isolated repository handle rather than sharing a single instance.Applies To: Background workers, parallel stream processors, and
DiffExecutorimplementations interacting with JGitRepositoryinstances.Why: Historically, reusing a single
Repositoryhandle across multiple threads during parallel cache generation caused unpredictable speed-ups, race conditions, and corrupted object reads. Failing to adhere to this typically results in Race Condition / State Corruption.
Trap 1: Passing a single shared JGit Repository instance to multiple asynchronous worker threads.
Don't:
Repository sharedRepo = repoManager.openRepository(project);
for (FileDiffCacheKey key : keys) {
executor.submit(() -> computeDiff(sharedRepo, key));
}
Do:
for (FileDiffCacheKey key : keys) {
executor.submit(() -> {
try (Repository workerRepo = repoManager.openRepository(project)) {
return computeDiff(workerRepo, key);
}
});
}
Context: This chapter governs the serialization and schema evolution strategies for persisting Gerrit change metadata to Git notes (NoteDb). It strictly enforces two-step schema rollouts, decoupled transfer objects, and permissive JSON parsing to guarantee data integrity across distributed cluster upgrades.
| Rule ID | Principle / | Priority | Primary Symptom / Trap | : : Constraint : : : | :-------- | :----------------- | :------- | :------------------------------- | | T2-01 | Two-Step Rollouts | Critical | Updating reading and writing | : : for Cached : : logic simultaneously without : : : Serialization : : accounting for mixed-version : : : Schemas : : cluster environments. : | T2-02 | Forward-Compatible | High | Deploying a strict JSON parser | : : NoteDb JSON : : that throws exceptions upon : : : Parsing : : encountering undocumented : : : : : fields. : | T2-03 | Decoupling | High | Serializing an internal database | : : Persistent Storage : : representation straight to a Git : : : Format from Legacy : : storage blob. : : : DB Entities : : : | T2-04 | Push Certificate | High | Using negative boolean logic to | : : Isolation by : : conditionally process metadata : : : Comment Status : : that only applies to specific : : : : : data domains. : | T2-05 | Industry-Standard | Medium | Using non-standard slang to | : : Terminology for : : differentiate older formatting : : : Deprecated Logic : : methods from modern JSON ones. :
Rule: Always execute a two-step rollout and explicitly increment the cache version when mutating persistent cache schemas.
What: Schema mutations in persistent caches must follow a two-step rollout: Step 1 introduces reader support for the new schema while continuing to write the old schema. Step 2 enables the new writer logic. A cache version increment is required.
Applies To: NoteDb serialization,
ChangeNotesState,ChangeNotesCache.Why: If a schema change is deployed without a two-step rollout, older instances of the application during a rolling restart will encounter cache records written by updated instances. Unable to parse the new schema, the older binaries will crash or drop data. Failing to adhere to this typically results in Deserialization Crash.
Trap 1: Updating reading and writing logic simultaneously without accounting for mixed-version cluster environments.
Don't:
Do:
Rule: Must configure JSON parsers to be inherently permissive and safely ignore unknown fields when deserializing NoteDb revision notes.
What: JSON parsers reading NoteDb revision notes must be inherently permissive, safely ignoring unknown fields to ensure system resilience against future metadata schema expansions.
Applies To: NoteDb deserialization algorithms parsing Git blob byte arrays into JSON objects.
Why: When introducing a new JSON-based payload format for persistent Gerrit metadata, strict parsing risked immediately breaking older service instances or external consumers whenever a new, valid metadata field was appended to the payload. Failing to adhere to this typically results in Parsing Exception.
Trap 1: Deploying a strict JSON parser that throws exceptions upon encountering undocumented fields.
Don't:
// BAD: Strict parsing crashes on new schema properties
Gson strictGson = new GsonBuilder().create();
RevisionNoteData data = strictGson.fromJson(reader, RevisionNoteData.class);
Do:
// GOOD: Permissive parsing explicitly ignoring unknown fields + regression tests verifying this behavior
Gson permissiveGson = new GsonBuilder().create();
RevisionNoteData data = permissiveGson.fromJson(reader, RevisionNoteData.class);
Rule: Never directly serialize legacy relational database objects into Git persistent storage.
What: Data models persisted to Git (NoteDb) must utilize dedicated, format-specific DTOs rather than directly serializing legacy relational database objects.
Applies To: NoteDb JSON Serialization pipelines and storage layer architecture.
Why: Initially, legacy ReviewDb models were mapped directly into JSON payloads. This inherited awkward relational quirks (e.g., missing critical keys that were traditionally injected at runtime) and locked the new storage schema to the constraints of an outgoing SQL-based design. Failing to adhere to this typically results in Schema Lock-In.
Trap 1: Serializing an internal database representation straight to a Git storage blob.
Don't:
// BAD: Serializing legacy database class
PatchLineComment comment = db.getComment();
gson.toJson(comment, outputStream);
Do:
// GOOD: Mapping to a decoupled storage DTO
RevisionNoteData.Comment noteComment = new RevisionNoteData.Comment(comment, serverId);
gson.toJson(noteComment, outputStream);
Rule: Must strictly map and extract push certificates exclusively when evaluating PUBLISHED comment records.
What: Push certificates must be strictly mapped and extracted only when evaluating PUBLISHED comment records, as they inherently do not exist in the isolated user refs storing DRAFT data.
Applies To: NoteDb revision note parsing logic handling cryptographic push metadata extraction.
Why: Push certificates are persisted exclusively in the main change meta ref. Draft comments are inherently personal and physically stored in isolated refs within
All-Users. Parsing logic initially failed to explicitly connect the certificate to the published context, risking logically malformed data associations. Failing to adhere to this typically results in Metadata Corruption.
Trap 1: Using negative boolean logic to conditionally process metadata that only applies to specific data domains.
Don't:
if (!draftsOnly) {
pushCert = parsePushCert(changeId, raw, p);
}
Do:
if (status == PatchLineComment.Status.PUBLISHED) {
// Certificates exist strictly in the published change context
pushCert = parsePushCert(changeId, raw, p);
} else {
pushCert = null;
}
Rule: Always use standard terminology like 'legacy' to designate older, decoupled schema formats.
What: Code paths, variables, and methods handling older, decoupled schema formats must use standard terminology like 'legacy' rather than informal slang.
Applies To: NoteDb schema evolution, serialization backwards-compatibility layers.
Why: Engineers occasionally used informal suffixes (like 'Homebrew') to designate older serialization logic, which obscured the architectural intent for developers unfamiliar with the slang. Failing to adhere to this typically results in Maintainability Degradation.
Trap 1: Using non-standard slang to differentiate older formatting methods from modern JSON ones.
Don't:
private void buildNoteHomebrew(ChangeNoteUtil noteUtil, OutputStream out) {
noteUtil.buildNote(buildCommentMap(), out);
}
Do:
private void buildNoteLegacy(ChangeNoteUtil noteUtil, OutputStream out) {
noteUtil.buildNote(buildCommentMap(), out);
}
Context: This chapter governs the configuration, evaluation, and strict syntactic enforcement of query predicates within the repository engine. It focuses on decoupling strict user-facing evaluation constraints from background observability and implementing phased, non-blocking rollouts to prevent configuration deadlocks.
| Rule ID | Principle / Constraint | Priority | Primary Symptom / | : : : : Trap : | :-------- | :-------------------------------- | :------- | :---------------- | | T3-01 | Non-Contributor Label Query | Medium | Blanket-excluding | : : Isolation : : the 'owner' of : : : : : the change from : : : : : acting as a : : : : : reviewer, even if : : : : : someone else : : : : : authored and : : : : : uploaded the : : : : : specific patchset : : : : : being reviewed. : | T3-02 | Phased Rollout of Strict Query | Critical | Using a single | : : Operator Enforcement : : feature toggle to : : : : : immediately : : : : : reject bad syntax : : : : : across the entire : : : : : system. : | T3-03 | Memoization of Static | Medium | Querying the | : : Configuration in Query Evaluators : : global : : : : : configuration : : : : : object repeatedly : : : : : within the core : : : : : evaluation loop. : | T3-04 | Metrics Collection Decoupling | High | Passing global | : : from Strict Query Constraints : : strict-mode : : : : : configuration : : : : : booleans into : : : : : metrics-gathering : : : : : query builders, : : : : : thereby enforcing : : : : : user-facing rules : : : : : on background : : : : : telemetry. :
Rule: Always explicitly exclude the uploader, committer, and author of the current patchset when evaluating
non_contributorqueries, allowing the change owner to act as an independent reviewer if they did not author the specific patchset.What: When evaluating label queries for
non_contributor, the query engine must explicitly exclude votes from the uploader, committer, and author of the current patchset, treating the change owner as a valid independent reviewer only if they didn't author/upload the specific patchset.Applies To: Query Engine (
EqualsLabelPredicate.java), Access Control Policies.Why: To enforce the 'four-eyes' principle, the system needed a way to distinguish between a change's code contributors and independent reviewers. The logic explicitly handles cases where the owner and uploader are different people, allowing the owner to act as an independent reviewer. Failing to adhere to this typically results in Policy Bypass / Self-Approval.
Trap 1: Blanket-excluding the 'owner' of the change from acting as a reviewer, even if someone else authored and uploaded the specific patchset being reviewed.
Don't:
// BAD: Excluding owner regardless of patchset authorship
if (account.equals(NON_CONTRIBUTOR_ACCOUNT_ID)) {
if (approver.equals(cd.getOwner())) return false;
}
Do:
// GOOD: Excluding only the uploader, committer, and author of the current patchset
if (account.equals(NON_CONTRIBUTOR_ACCOUNT_ID)) {
if (uploader.equals(approver) || committer.equals(approver) || author.equals(approver)) {
return false;
}
}
Rule: Never use a single feature toggle to instantly enforce new query syntax constraints; you must decouple read and write configurations to allow manual migrations.
What: Breaking syntactic requirements for repository queries must be rolled out via decoupled read/write configuration flags to avoid circular dependency deadlocks.
Applies To: Submit Requirements evaluation and configuration validation layers.
Why: Instantly enforcing strict query syntax (removing default searches) would break legacy submit configurations. Because a broken configuration blocks all repository submissions, teams would be permanently locked out from submitting the code necessary to fix the broken configuration. Failing to adhere to this typically results in Repository Deadlock.
Trap 1: Using a single feature toggle to immediately reject bad syntax across the entire system.
Don't:
Do:
requireOperatorForUpdate (blocks
saving new bad configs) and requireOperatorForEvaluation (throws errors
dynamically). Enable the update block first, migrate legacy rules manually,
then enable evaluation blocking.Rule: Must cache query parsing and evaluation configurations during component initialization instead of polling the configuration object per execution.
What: Configuration values that govern query parsing and evaluation must be read once during component initialization rather than fetched dynamically per execution.
Applies To: Query Builders, Evaluators (e.g., SubmitRequirementsEvaluatorImpl), and high-frequency validation execution paths.
Why: Historically, reading configuration values directly from the Config object upon every expression validation or evaluation incurred unnecessary CPU overhead, degrading the performance of high-volume operations like submit requirement checks. Failing to adhere to this typically results in Performance Degradation.
Trap 1: Querying the global configuration object repeatedly within the core evaluation loop.
Don't:
public SubmitRequirementExpressionResult evaluateExpression(
SubmitRequirementExpression expression, ChangeData changeData) {
// BAD: Reading config on every evaluation
boolean reqOp = config.getBoolean("submit-requirement", null, "requireOperatorForEvaluation", false);
Predicate<ChangeData> predicate = queryBuilderFactory.create(reqOp).parse(expression.expressionString());
// ...
}
Do:
// GOOD: Cache the configuration value at initialization time
public SubmitRequirementsEvaluatorImpl(...) {
this.requireOperatorForEvaluation = config.getBoolean("submit-requirement", null, "requireOperatorForEvaluation", false);
}
public SubmitRequirementExpressionResult evaluateExpression(
SubmitRequirementExpression expression, ChangeData changeData) {
Predicate<ChangeData> predicate = queryBuilderFactory.create(this.requireOperatorForEvaluation).parse(expression.expressionString());
// ...
}
Exceptions: Dynamic configurations explicitly designed and documented to be hot-reloaded without server restarts.
Rule: Always hard-code permissive configuration for metrics-gathering systems to prevent observability failures caused by malformed user metadata.
What: Metrics gathering systems must bypass strict, user-facing query validation configurations to guarantee that observability pipelines do not fail due to malformed metadata or legacy data.
Applies To: Metrics exporters (e.g., MergeMetrics), Background Jobs, and Observability Hooks.
Why: Metrics collection for change merging relied on global server configuration for query parsing. If strict operator evaluation was enabled globally, legacy or malformed submit requirements would cause the metrics collection to throw an exception, halting the entire telemetry pipeline. Failing to adhere to this typically results in Metrics Data Loss.
Trap 1: Passing global strict-mode configuration booleans into metrics-gathering query builders, thereby enforcing user-facing rules on background telemetry.
Don't:
// BAD: Tying metrics collection to strict user configurations
boolean strictEvaluation = config.getBoolean("submit-requirement", null, "requireOperatorForEvaluation", false);
Predicate<ChangeData> predicate = submitRequirementChangequeryBuilderFactory
.create(strictEvaluation)
.parse(submitRequirement.submittabilityExpression());
Do:
// GOOD: Hard-code permissive configuration to guarantee metrics stability
Predicate<ChangeData> predicate = submitRequirementChangequeryBuilderFactory
.create(false) // Hardcoded leniency
.parse(submitRequirement.submittabilityExpression());
Context: This chapter governs the evaluation and auditing of impersonated
access operations (e.g., SUBMIT_AS and RUN_AS). It defines strict mechanisms
for isolating the initiating "real user" from the "impersonated user" during
dynamic permission checks, and mandates automated, centralized logging in NoteDb
and Protobuf to guarantee uncorrupted audit trails.
| Rule ID | Principle / Constraint | Priority | Primary Symptom / Trap |
|---|---|---|---|
| T4-01 | Impersonation Log | Medium | Allowing an impersonated |
| : : Suppression for Reviewer : : session to generate a : | |||
| : : Updates : : change log message for a : | |||
| : : : : reviewer addition. : | |||
| T4-02 | Strict Context Evaluation | Critical | Checking for SUBMIT_AS |
| : : for Impersonated Access : : within a pre-calculated : | |||
| : : Control : : collection of permissions : | |||
| : : : : belonging to the user : | |||
| : : : : context being : | |||
| : : : : impersonated. : | |||
| T4-03 | Centralized Impersonation | High | Appending the |
| : : Auditing in NoteDb : : impersonation string : | |||
| : : Commits : : manually in specific API : | |||
| : : : : endpoints. : | |||
| T4-04 | System Metadata | High | Manually mutating the |
| : : Preservation During : : change message to inject : | |||
| : : Impersonation Updates : : audit metadata during a : | |||
| : : : : batch update operation. : | |||
| T4-05 | Explicit Distinction of | Medium | Adding tracking fields to |
| : : Effective vs Real : : Protobuf messages without : | |||
| : : Identity in Protobuf : : clarifying the : | |||
| : : Schemas : : impersonation : | |||
| : : : : relationship. : |
Rule: Always suppress system-generated impersonation change messages during operations that strictly update reviewers to avoid redundant logs.
What: System-generated impersonation change messages must be suppressed for operations that explicitly update reviewers, as reviewer updates inherently persist their own metadata.
Applies To: Gerrit server API, specifically
AddReviewersOp.javaand operations modifyingReviewerUpdateInfo.Why: Operations like adding a reviewer do not generate standard change messages by default, relying instead on structured reviewer-update lists. Allowing impersonation logic to inject a change message for these operations created redundant, noisy log entries. Failing to adhere to this typically results in Audit Log Noise.
Trap 1: Allowing an impersonated session to generate a change log message for a reviewer addition.
Don't:
change = ctx.getChange();
if (!accountIds.isEmpty()) {
// Proceeds with update, implicitly allowing impersonation messages
}
Do:
change = ctx.getChange();
// Reviewer updates do not create change messages. In case of impersonation, we do not want to add an extra message to the log.
ctx.getUpdate(change.currentPatchSetId()).setSuppressImpersonationMessage(true);
if (!accountIds.isEmpty()) {
// Proceeds with update
}
Rule: Never authorize impersonated actions against the bulk permission set of the impersonated user; always evaluate against the real user initiating the impersonation.
What: When authorizing impersonated actions (e.g.,
SUBMIT_AS), permissions must be verified against theREAL_USER(the identity initiating the impersonation) rather than checking the bulk permission set of the impersonated user.Applies To: Access control layers,
PermissionBackend, and operational checkpoints likeMergeOp.java.Why: Evaluating
SUBMIT_ASagainst the general permissions pool of the impersonated user context was conceptually flawed and risked allowing unauthorized users to execute privileged actions if the real user lacked explicit impersonation rights. Failing to adhere to this typically results in Security Bypass.
Trap 1: Checking for SUBMIT_AS within a pre-calculated collection of
permissions belonging to the user context being impersonated.
Don't:
// BAD: Checking against the overall 'can' permission set which may not represent the real user's explicit rights.
if (!can.contains(ChangePermission.SUBMIT_AS)) {
// reject
}
Do:
// GOOD: Permissions are checked against the user who initiated the impersonation (REAL_USER).
if (!permissionBackend
.user(caller, ImpersonationPermissionMode.REAL_USER)
.change(cd)
.test(ChangePermission.SUBMIT_AS)) {
// reject
}
Rule: Must centrally inject impersonation clauses directly into NoteDb commit builders to guarantee consistent audit trails across all paths.
What: Impersonation clauses (e.g., 'Performed by X on behalf of Y') must be automatically centralized within the NoteDb
AbstractChangeUpdatecommit builder, guaranteeing their presence in the system's versioned storage regardless of the operation or whether a user-provided message exists.Applies To: NoteDb mutation layers,
AbstractChangeUpdate.java, and all REST endpoints modifying change states.Why: Historically, impersonation messages were manually appended by individual REST API handlers (like
PostReview). This decentralized approach led to inconsistent audit trails where certain backend or automated operations failed to properly record the real user identity in the underlying Git commit. Failing to adhere to this typically results in Incomplete Audit Trail.
Trap 1: Appending the impersonation string manually in specific API endpoints.
Don't:
// BAD: Decentralized in PostReview.java
String impersonationClause = String.format("(Posted by %s on behalf of %s)", caller, reviewer);
if (Strings.isNullOrEmpty(in.message)) {
in.message = impersonationClause;
}
Do:
// GOOD: Centralized in AbstractChangeUpdate.java for all NoteDb writes
private void addOptionalImpersonationMessage(CommitBuilder cb) {
if (realAccountId == null || realAccountId.equals(accountId)) return;
String impersonationClause = String.format("(Performed by %s on behalf of %s)", realLoggableName, loggableName);
// Safely inject before the footer
}
Trap 2: Skipping the impersonation clause if the change update has no user-visible commit message body.
Don't:
int firstFooterLine = indexOfFirstFooterLine(commitMsgLines);
// BAD: Bailing out if the message has no body
if (firstFooterLine == 2) return;
Do:
// GOOD: Append the clause regardless of the message body length using standard string manipulation.
Stream.concat(Arrays.stream(commitMsgLines).limit(firstFooterLine), Stream.of(impersonationClause, ""))...
Exceptions: Operations where the realAccountId strictly equals the
accountId (no impersonation taking place).
Rule: Never manually mutate standard system change messages to document batch update reviewer modifications on behalf of other users.
What: Manual override of change messages must be avoided when making system-level reviewer updates on behalf of other users, as it corrupts or suppresses structured audit logs.
Applies To: REST API mutation endpoints, specifically batch updates involving user impersonation (
RUN_AS/onBehalfOf).Why: Adding a manual "on behalf of" message to a reviewer modification suppressed the default system message (e.g., "Bob added to reviewers"), wiping out the primary context. Furthermore, tests verifying impersonation broke because reviewer updates bypass standard visible change messages entirely. Failing to adhere to this typically results in Audit Trail Corruption.
Trap 1: Manually mutating the change message to inject audit metadata during a batch update operation.
Don't:
update.setChangeMessage(String.format("Reviewer added by %s on behalf of %s", realUser, impersonatedUser));
Do:
ReviewerUpdateInfo
automatically. Do not append manual strings to the batch update.Trap 2: Writing tests that assert against visible change messages to verify background auditing.
Don't:
ChangeMessage m = Iterables.getLast(cmUtil.byChange(r.getChange().notes()));
assertThat(m.getMessage()).contains(expectedReviewerName);
Do:
ChangeMessageInfo lastMessage = Iterables.getLast(gApi.changes().id(r.getChangeId()).get().messages);
assertThat(lastMessage.realAuthor._accountId).isEqualTo(realUser.id().get());
assertThat(lastMessage.author._accountId).isEqualTo(impersonatedUser.id().get());
Rule: Must explicitly document Protobuf schema fields storing impersonation audit trails with their exact
RUN_ASsemantics.What: Fields storing impersonation audit trails in Protobuf schemas must contain explicit inline documentation demonstrating exact
RUN_ASsemantics.Applies To: Protobuf schemas, particularly caching and status update models like
ReviewerStatusUpdateProto.Why: Ambiguity in the protobuf schema left developers unsure which field represented the impersonated account vs the actual administrative account performing the action. Failing to adhere to this typically results in Misinterpreted Audit Logs.
Trap 1: Adding tracking fields to Protobuf messages without clarifying the impersonation relationship.
Don't:
int32 updated_by = 2;
int32 real_updated_by = 8;
Do:
// Account ID of the effective user.
int32 updated_by = 2;
// Account ID of the real user. Set when impersonating using the RUN_AS permission.
// Example: if User X is impersonating user Y, real_updated_by is X.
int32 real_updated_by = 8;
Context: This theme defines constraints for handling deleted or recreated user accounts to prevent identity-cycling exploits, specifically isolating validation checks and filtering orphaned approvals during merge operations without degrading system performance.
| Rule ID | Principle / | Priority | Primary Symptom / Trap | : : Constraint : : : | :-------- | :---------------- | :------- | :-------------------------------- | | T5-01 | Invalidation of | Critical | Evaluating submittability by | : : Approvals from : : aggregating all present votes : : : Deleted Accounts : : without verifying if the account : : : : : IDs associated with those votes : : : : : still exist. : | T5-02 | Deferral of | High | Triggering cache lookups for | : : Expensive Account : : every account associated with a : : : Validation to : : change during standard page loads : : : Submission Time : : or SR recalculations. : | T5-03 | Graceful | High | Throwing a terminal exception as | : : Degradation for : : soon as an orphaned or invalid : : : Invalid Approval : : property is discovered during an : : : Entities : : aggregation check. : | T5-04 | Positive-Intent | Medium | Creating a flag that requires an | : : Feature Toggles : : administrator to explicitly turn : : : for : : on a necessary security patch. : : : Secure-by-Default : : : : : States : : :
Rule: Always dynamically filter and ignore
PatchSetApprovalvotes originating from deleted accounts during submission checks.What: Code review approvals (
PatchSetApprovalvotes) from deleted accounts must be dynamically filtered and ignored during submission checks to prevent identity-cycling exploits.Applies To: Submission validation (
MergeOp.checkSubmitRequirements), Submit Requirement evaluation, and IAM lifecycle processing.Why: An exploit vector was identified where a user could upload a patch, delete their Gerrit account, and recreate an account linked to the same external ID. The new account inherited external group permissions (e.g., Code-Review+2) and could self-approve the change made by the now-deleted identity, bypassing the 'no self-approval' rule. Failing to adhere to this typically results in Security Bypass / Self-Approval Exploit.
Trap 1: Evaluating submittability by aggregating all present votes without verifying if the account IDs associated with those votes still exist.
Don't:
for (PatchSetApproval psa : cd.currentApprovals()) {
if (psa.isApproved()) return true;
}
Do:
for (PatchSetApproval psa : filterOutApprovalsOfDeletedAccounts(cd.currentApprovals())) {
if (psa.isApproved()) return true;
}
Rule: Never execute costly cache lookups for account existence during continuous operations; must defer these validations to the final submission phase.
What: Costly verification checks, such as querying an
AccountCachefor account existence, must not be integrated into continuous operations like general Submit Requirement (SR) evaluation. They must be deferred to the finalMergeOpsubmission phase.Applies To: Submit Requirements engine and
MergeOp.Why: An initial attempt to fix a vulnerability by filtering deleted account votes inside the continuous Submit Requirements engine caused severe global latency regressions because
AccountCachewas queried excessively. The fix had to be moved strictly to the submit button execution path. Failing to adhere to this typically results in Severe Latency / System Degradation.
Trap 1: Triggering cache lookups for every account associated with a change during standard page loads or SR recalculations.
Don't:
accountCache.get(accountId).isPresent() inside the highly
trafficked Submit Requirement evaluation engine.Do:
MergeOp.checkSubmitRequirements() directly prior to the final merge
action.Rule: Always silently filter out invalid or orphaned entities on a resource rather than hard-failing the operation.
What: The presence of an invalid or orphaned entity (e.g., an approval from a deleted user) on a resource must not hard-fail operations on that resource. The invalid entity should be filtered out, and the operation allowed to proceed if the remaining valid entities satisfy the requirements.
Applies To: Access validation, Submit requirement evaluation, and Merge logic.
Why: A proposed solution suggested throwing a
ResourceConflictExceptionif any deleted account vote was detected on a change. This was rejected because it would block a change from being merged even if it possessed enough valid votes from active users to pass independently. Failing to adhere to this typically results in Unnecessary Operation Blocking.
Trap 1: Throwing a terminal exception as soon as an orphaned or invalid property is discovered during an aggregation check.
Don't:
if (isDeleted(account)) {
throw new ResourceConflictException("Approval made by deleted account");
}
Do:
// Silently filter invalid data and evaluate based on remaining valid data
Iterable<PatchSetApproval> validVotes = Iterables.filter(votes, v -> !isDeleted(v.accountId()));
Rule: Must name and implement feature toggles for security fixes such that the insecure legacy behavior requires an explicit opt-in.
What: When introducing a critical security or architectural fix guarded by a feature toggle, the toggle's name and logic must represent the opt-in of the legacy/insecure behavior, ensuring the new secure behavior is the unconfigured system default.
Applies To: Experiment Flag definitions (
ExperimentFeaturesConstants) and Feature Toggle conditionals.Why: A security patch introduced a flag named
IGNORE_VOTES_OF_DELETED_ACCOUNTS. Code reviewers forced an inversion of the flag toCONSIDER_VOTES_OF_DELETED_ACCOUNTSso that administrators did not have to explicitly enable the fix, preventing unpatched defaults. Failing to adhere to this typically results in Accidental Misconfiguration / Security Hole.
Trap 1: Creating a flag that requires an administrator to explicitly turn on a necessary security patch.
Don't:
if (experimentFeatures.isFeatureEnabled("IGNORE_VOTES_OF_DELETED_ACCOUNTS")) {
filterDeletedAccounts();
}
Do:
if (!experimentFeatures.isFeatureEnabled("CONSIDER_VOTES_OF_DELETED_ACCOUNTS")) {
filterDeletedAccounts();
}
Context: REST API endpoints must strictly reflect hierarchical entity relationships while optimizing JSON payloads to minimize bandwidth and processing latency. Modifications to serialization pipelines must explicitly gate expensive permission checks and gracefully handle redundant or experimental fields without causing UI ambiguity.
| Rule ID | Principle / Constraint | Priority | Primary Symptom / Trap |
|---|---|---|---|
| T6-01 | Explicit Boolean | Medium | Using a toBoolean |
| : : Rendering for : : helper that converts : | |||
: : Experimental UI Features : : false to null to : | |||
| : : : : minimize payload size, : | |||
| : : : : unintentionally leaving : | |||
| : : : : the UI client ambiguous : | |||
| : : : : about whether the feature : | |||
| : : : : is disabled or simply : | |||
| : : : : unconfigured. : | |||
| T6-02 | Hierarchical REST API | Medium | Exposing a sub-resource |
| : : Resource Routing : : directly under a : | |||
| : : : : high-level container : | |||
| : : : : because it seems more : | |||
| : : : : direct. : | |||
| T6-03 | Explicit Query Parameters | Medium | Defaulting a new diff |
| : : for Extensible Listing : : endpoint to return only : | |||
| : : APIs : : file names without : | |||
| : : : : requiring the client to : | |||
| : : : : ask for that specific : | |||
| : : : : format. : | |||
| T6-04 | Performance Justification | High | Appending a permission |
| : : for Payload-Embedded : : verification step to : | |||
| : : Permission Checks : : every response object : | |||
| : : : : regardless of the : | |||
| : : : : client's actual need for : | |||
| : : : : that data. : | |||
| T6-05 | Omission of Redundant | Medium | Falling back to the |
| : : Real-User Data in API : : primary user object if a : | |||
| : : Payloads : : distinct real user isn't : | |||
| : : : : found. : |
Rule: Always explicitly serialize
falsefor experimental UI feature flags to ensure strict UI gating, temporarily bypassing standard payload nullification.What: REST API responses should temporarily bypass the standard payload optimization (which drops
falseandnullvalues) for experimental UI feature flags, explicitly returningfalseto ensure strict UI gating.Applies To: REST API JSON serializers (
ChangeJson), UI Feature Flags.Why: To prevent an experimental AI review feature from leaking in the UI when permission was denied, the API was modified to explicitly serialize the
canAiReview = falsestate rather than omitting it as the framework usually does for falsy values. Failing to adhere to this typically results in UI Feature Leakage.
Trap 1: Using a toBoolean helper that converts false to null to minimize
payload size, unintentionally leaving the UI client ambiguous about whether the
feature is disabled or simply unconfigured.
Don't:
// BAD: Omitting false values from payload
info.canAiReview = toBoolean(permissionBackend.test(AI_REVIEW));
Do:
// GOOD: Explicitly forcing false to be serialized
info.canAiReview = permissionBackend.test(AI_REVIEW) ? true : false;
Exceptions: This is a temporary technical debt exception strictly permitted
until the experiments.UiFeature__enable_ai_chat feature flag is sunsetted.
Rule: Never expose sub-resources at the root level; always nest them under their specific context-defining parent resources.
What: REST API endpoints must reflect strict hierarchical entity relationships. Sub-resources must be nested under their specific context-defining parent resources rather than being exposed at the root level.
Applies To: REST API Controller definitions, API endpoint URL schemas, and resource mapping.
Why: Designing flat API endpoints (e.g., requesting a file diff directly from the project root) created ambiguities, as the backend could not definitively validate the file's existence without knowing the specific commit context. Failing to adhere to this typically results in Ambiguous Resource Resolution.
Trap 1: Exposing a sub-resource directly under a high-level container because it seems more direct.
Don't:
// BAD: Missing the commit context required to resolve the file
GET /projects/{project}/files/{file}/diff?old={sha1}&new={sha1}
Do:
// GOOD: Nesting the file diff under the explicit commit it belongs to
GET /projects/{project}/commits/{commit-id}/files/{file}/diff?base={sha1}
Rule: Must require an explicit query parameter for REST endpoints designed to return sparse payloads to preserve future backwards-compatibility.
What: When creating a REST API endpoint that deliberately returns a sparse payload (e.g., listing only file names), it must require an explicit query parameter indicating that reduced scope, allowing future backwards-compatible payload expansions.
Applies To: REST API Endpoint design, list views, and differential payload returns.
Why: Endpoints originally designed to return sparse data became locked into that format. When full data payloads were later needed, developers had to create entirely new endpoints because the default behavior could not be safely changed. Failing to adhere to this typically results in API Backward Incompatibility.
Trap 1: Defaulting a new diff endpoint to return only file names without requiring the client to ask for that specific format.
Don't:
// BAD: Locks the API into only ever returning filenames
GET /projects/{project}/commits/{commit-id}/diff
Do:
// GOOD: Requires the client to acknowledge the limited scope
GET /projects/{project}/commits/{commit-id}/diff?nameOnly
Exceptions: Endpoints whose domain definition inherently restricts them to
simple lists (e.g., GET /ids).
Rule: Always gate newly injected backend permission evaluations in heavily trafficked JSON payloads behind explicit client options to prevent global latency degradation.
What: Injecting new backend permission evaluations into heavily trafficked REST API JSON payloads must be critically analyzed against the latency penalty and explicitly gated by granular request options.
Applies To: REST API response serializers, specifically
ChangeJsonformatting pipelines.Why: Adding mandatory permission checks (like AI_REVIEW) to core change listing APIs increased the time complexity of the request for all users, introducing latency that could have been avoided by using a separate API or a request parameter gate. Failing to adhere to this typically results in API Latency Degradation.
Trap 1: Appending a permission verification step to every response object regardless of the client's actual need for that data.
Don't:
// BAD: Unconditional permission check slows down all queries
out.canAiReview = permissionBackend.user(user).test(AI_REVIEW);
Do:
// GOOD: Gating the expensive check behind explicit client options and experiment flags
if (has(CURRENT_ACTIONS) && experiments.isEnabled(ENABLE_AI_CHAT)) {
out.canAiReview = permissionBackend.user(user).test(AI_REVIEW);
}
Rule: Must omit the
real_updated_byfield (set to null) when serializing objects if it is identical to the primaryupdated_byfield to conserve bandwidth.What: When serializing objects that support impersonation (like
ReviewerUpdateInfo), thereal_updated_byfield must be omitted (set to null) if it is identical to theupdated_byfield, rather than duplicating the payload data.Applies To: REST API serialization layers, specifically
ChangeJson.javaand TypeScript interface definitions.Why: Returning the same account identity for both the primary actor and the real actor bloated the JSON payload. By omitting it, the API clearly signals when impersonation has not occurred while conserving bandwidth. Failing to adhere to this typically results in Payload Bloat.
Trap 1: Falling back to the primary user object if a distinct real user isn't found.
Don't:
// BAD: Duplicating the object if no distinct real user exists
new ReviewerUpdateInfo(
c.date(),
accountLoader.get(c.updatedBy()),
c.realUpdatedBy().map(accountLoader::get).orElseGet(() -> accountLoader.get(c.updatedBy())),
...
Do:
// GOOD: Return null to omit the field entirely
new ReviewerUpdateInfo(
c.date(),
accountLoader.get(c.updatedBy()),
c.realUpdatedBy().map(accountLoader::get).orElse(null),
...
Context: This chapter governs the configuration, versioning, and migration
of the build system infrastructure, focusing heavily on Bazel modules (bzlmod)
and dependency graphs. It establishes constraints to ensure strict version
alignment, prevent compliance pipeline stalls, and maintain reliable developer
bootstrapping environments.
| Rule ID | Principle / Constraint | Priority | Primary Symptom / Trap |
|---|---|---|---|
| T7-01 | Bazelisk Version | High | Relying on deprecated |
| : : Constraints for Bzlmod : : Bazelisk versions while : | |||
| : : Migrations : : altering legacy Bazel : | |||
| : : : : boundary markers : | |||
: : : : (WORKSPACE). : | |||
| T7-02 | Strict Version Alignment | High | Pinning a specific older |
| : : in Bazel Dependency : : minor version of a library : | |||
| : : Graphs : : for a single module : | |||
| : : : : without checking global or : | |||
| : : : : internal mirror alignment. : | |||
| T7-03 | Synchronized Dependency | Medium | Deprecating a build |
| : : Manifest Documentation : : configuration file while : | |||
| : : : : leaving stale references : | |||
| : : : : to its usage in textual : | |||
| : : : : documentation. : | |||
| T7-04 | String Typing for Bazel | High | Passing a native boolean |
: : amend_artifact : : True to a strictly : | |||
| : : force_version : : string-typed attribute in : | |||
| : : : : a Bazel macro. : | |||
| T7-05 | Segregation of External | Medium | Placing non-internal |
| : : Dependencies for : : dependencies alongside : | |||
| : : Compliance Bypassing : : standard organizational : | |||
| : : : : dependencies in global : | |||
| : : : : dependency files. : |
Rule: Always upgrade
bazeliskto at least v1.27.0 when migrating tobzlmodand removing or renaming the rootWORKSPACEfile.What: When migrating to Bazel Modules (
bzlmod) and removing or renaming the rootWORKSPACEfile,bazeliskmust be upgraded to at least v1.27.0 to correctly detect the.bazelversionfile.Applies To: Build System / CI pipelines / Developer environment bootstrapping.
Why: During the migration to Bzlmod, the
WORKSPACEfile was renamed toWORKSPACE.bzlmod. Older versions ofbazeliskfailed to locate the project root without a file strictly namedWORKSPACE, ignoring the pinned.bazelversionand downloading the newest incompatible Bazel binary (e.g., v9.0.1). Failing to adhere to this typically results in Build Failure / Incorrect Toolchain.
Trap 1: Relying on deprecated Bazelisk versions while altering legacy Bazel
boundary markers (WORKSPACE).
Don't:
bazelisk
that defaults to Bazel 9.x when WORKSPACE is missing.Do:
bazelisk >= 1.27.0 which resolves .bazelversion even in purely
bzlmod-driven repositories lacking a WORKSPACE file.Rule: Must explicitly align external JVM dependencies to a single validated minor version across the entire build graph.
What: External JVM dependencies must strictly align with a single validated minor version to prevent build system conflicts in
rules_jvm_externaland ensure compatibility with internal monolithic dependency mirrors.Applies To: Bazel build configurations,
WORKSPACE, anddeps.tomldefining external library versions.Why: Introducing conflicting minor versions of transitive libraries (e.g., pulling Bytebuddy 1.18.4 while another module expects 1.18.5) caused the build system to trigger duplicate version checks and fail the build graph resolution. Failing to adhere to this typically results in Build Failure / Duplicate Version.
Trap 1: Pinning a specific older minor version of a library for a single module without checking global or internal mirror alignment.
Don't:
bytebuddy:1.18.4 in the JGit servlet dependency chain while the
rest of the build relies on 1.18.5.Do:
bytebuddy:1.18.5, ensuring a single version traverses the entire
graph.Rule: Always update developer documentation atomically in the exact same commit when modifying or deprecating dependency manifests.
What: Build system developer documentation must be atomically updated in the exact same commit whenever dependency declaration manifests or build targets are migrated or deprecated.
Applies To: Build system configuration (e.g., Bazel WORKSPACE/MODULE files) and corresponding developer-facing documentation (e.g., dev-bazel.txt).
Why: During a migration of dependency management workflows, the legacy dependency configuration file was emptied, but the developer documentation still explicitly instructed engineers to reference the deprecated file path and run outdated Bazel pin targets. Failing to adhere to this typically results in Developer Process Failure.
Trap 1: Deprecating a build configuration file while leaving stale references to its usage in textual documentation.
Don't:
tools/deps.bzl) but failing to
update documentation containing old execution targets like bazel run @gerrit_deps//:pin.Do:
tools/deps.toml) and correctly mapped targets like bazel run @external_deps//:pin within the deprecation commit.Rule: Never pass a Starlark boolean to the
force_versionattribute inrules_jvm_external; it must be strictly typed as a string.What: The
force_versionattribute within therules_jvm_externalBazel module extension must be strictly passed as a string, not a native Starlark boolean.Applies To: Bazel build scripts (
MODULE.bazel) leveraging therules_jvm_externalextension for Maven dependency resolution.Why: An attempt was made to enforce root-level dependency precedence over layered modules using a boolean data type. The underlying extension API structurally requires a string representation of the boolean value. Failing to adhere to this typically results in Bazel Evaluation Failure.
Trap 1: Passing a native boolean True to a strictly string-typed attribute
in a Bazel macro.
Don't:
maven.amend_artifact(
name = "external_deps",
coordinates = coord,
force_version = True,
)
Do:
maven.amend_artifact(
name = "external_deps",
coordinates = coord,
force_version = "true",
)
Rule: Must isolate external dependencies into dedicated configuration files to avoid triggering unnecessary internal compliance reviews.
What: Dependencies not utilized internally by the host organization must be isolated into dedicated build configurations to avoid triggering unnecessary compliance reviews.
Applies To: Bazel workspace configurations, dependency management (e.g., deps.bzl, nongoogle.bzl).
Why: Routine version bumps to open-source libraries (e.g., H2 database) that were only used in external distributions triggered mandatory internal 'Library-Compliance' votes, blocking development velocity and complicating functional PRs. Failing to adhere to this typically results in Build Pipeline Stalls.
Trap 1: Placing non-internal dependencies alongside standard organizational dependencies in global dependency files.
Don't:
tools/deps.bzl file
alongside core infrastructure libraries.Do:
tools/nongoogle.bzl file
to explicitly demarcate its exclusion from internal compliance checks.Trap 2: Bundling dependency location moves with functional, logical code changes.
Don't:
Do:
nongoogle.bzl as an independent parent
change to keep the functional logic updates completely decoupled.Exceptions: Dependencies that share utilization across both internal infrastructure and external deployments.
Context: Asynchronous notification pipelines must capture exact entity state at the moment of initialization rather than fetching it dynamically during execution. This guarantees data consistency and prevents delayed background tasks from inadvertently processing future, out-of-band updates from persistent storage.
| Rule ID | Principle / Constraint | Priority | Primary Symptom / Trap |
|---|---|---|---|
| T8-01 | Snapshot State Capture | High | Passing database identifiers |
| : : for Asynchronous : : to a background task and : | |||
| : : Notifications : : reloading the entity during : | |||
| : : : : execution. : |
Rule: Must materialize and pass the exact entity state synchronously to asynchronous background tasks before handoff.
What: Asynchronous tasks (like email dispatchers) must capture the exact entity state at initialization instead of fetching it from the persistent store dynamically during execution.
Applies To: Asynchronous notification pipelines and background task executors.
Why: A delayed asynchronous email task would query NoteDb directly during execution. If another thread performed an update during the delay, the email would accidentally pull 'too-new' state (e.g., a mismatched subject line) that belonged to the subsequent transaction. Failing to adhere to this typically results in Race Condition / Stale Data.
Trap 1: Passing database identifiers to a background task and reloading the entity during execution.
Don't:
public ChangeEmailImpl(@Provided EmailArguments args, Project.NameKey project, Change.Id changeId) {
// Anti-pattern: Storing IDs and reading the database asynchronously later
this.changeId = changeId;
}
Do:
public ChangeEmailImpl(@Provided EmailArguments args, Change change) {
// Materialize state synchronously before handing off to the async thread
this.changeData = args.changeDataFactory.create(change);
this.change = changeData.change();
}
Context: This chapter dictates the implementation of parameterized integration tests, strict Guice dependency injection, and granular flaky test isolation. It ensures deterministic test coverage across multiple backend configurations without leaking generic APIs or brittle global states.
| Rule ID | Principle / Constraint | Priority | Primary Symptom / Trap |
|---|---|---|---|
| T9-01 | Method-Level Granularity | Medium | Using class-level |
| : : for Integration Test : : isolation to fix a single : |