agents: Add CRAG-generated review agents and skills With cl/930783235, this change sets up two AI review agents that run automatically on new changes. Bug: 522929179 Change-Id: I315a7ec327dd30af842ec890818cf697756fd55c Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/596721 Commit-Queue: Gavin Mak <gavinmak@google.com> Reviewed-by: Mike Frysinger <vapier@google.com> Tested-by: Gavin Mak <gavinmak@google.com>
diff --git a/agents/README.md b/agents/README.md new file mode 100644 index 0000000..f2e836a --- /dev/null +++ b/agents/README.md
@@ -0,0 +1,42 @@ +# git-repo AI Review Agents (WIP) + +**Note:** This project is a work in progress and is subject to change. + +This directory contains configurations and skills for AI review agents that +automatically analyze changes in the `git-repo` codebase. + +These agents help maintain code quality, enforce style guidelines, and catch +common pitfalls before code is merged. + +## Directory Structure + +* [`agent_configs.txtpb`](agent_configs.txtpb): Defines the active AI + agents, their configurations, and which skills they are equipped with. +* [`skills/`](skills/): Contains the "skills" (rules, guidelines, and traps) + used by the agents. + * [`code_review_workflow/`](skills/code_review_workflow/SKILL.md): + Guidelines for code review processes, commit messages, and testing. + * [`core_internals/`](skills/core_internals/SKILL.md): Technical + guidelines for `git-repo` core logic (sync, manifest, git integration, + etc.). + +## How It Works + +The agents defined in `agent_configs.txtpb` are configured to run +automatically on new changes. They analyze the diffs against the rules defined +in their respective skills and provide feedback in the code review interface +(e.g., Gerrit). + +## Contributing + +To improve the agent's review quality or add new rules: + +1. **Update existing skills**: Modify the `SKILL.md` files under `skills/` to + add new rules, "What" explanations, "Why" rationales, and "Traps" + (Don't/Do code examples). +2. **Add new skills**: + * Create a new directory under `skills/`. + * Add a `SKILL.md` following the established format (see existing skills + for reference). + * Register the new skill in `agent_configs.txtpb` by adding it to the + `skills` field of an agent configuration.
diff --git a/agents/agent_configs.txtpb b/agents/agent_configs.txtpb new file mode 100644 index 0000000..80ac023 --- /dev/null +++ b/agents/agent_configs.txtpb
@@ -0,0 +1,26 @@ +# proto-file: google/corp/android/engprod/codereviewagentconfiguration/v1/agent.proto +# proto-message: HostAgents + +# Code Review Workflow Agent +configs { + id: "code-review-workflow" + display_name: "Code Review Workflow" + description: "Analyzes git-repo Gerrit submission labeling, commit metadata, Python linting, and testing strategy." + skills: "code_review_workflow" + include_filters { + project: "git-repo" + } + automatic: true +} + +# Core Internals Agent +configs { + id: "core-internals" + display_name: "Core Internals" + description: "Analyzes git-repo synchronization, multiprocessing, manifest parsing, git integration, worktree layouts, and CLI commands." + skills: "core_internals" + include_filters { + project: "git-repo" + } + automatic: true +}
diff --git a/agents/skills/code_review_workflow/SKILL.md b/agents/skills/code_review_workflow/SKILL.md new file mode 100644 index 0000000..71d0ed2 --- /dev/null +++ b/agents/skills/code_review_workflow/SKILL.md
@@ -0,0 +1,968 @@ +--- +name: code-review-workflow +description: Provides guidance and best practices on Gerrit submission labeling, CI builder execution, Python code formatting/linting, commit metadata standardization, and testing strategy in git-repo. +--- + +# Code Review Workflow Engineering Guide + +## Executive Summary + +Welcome to the authoritative engineering guide for the Code Review Workflow. +This living repository exists to capture critical folk knowledge, prevent the +recurrence of historical failure modes, and enforce strict architectural and +procedural boundaries across our integration pipeline. By standardizing these +protocols, we ensure high development velocity while maintaining rock-solid +codebase stability and traceability. + +This guide covers the complete lifecycle of a change list (CL) from local +development to automated submission. It defines the strict Gerrit labeling +mechanisms required to trigger the Commit-Queue, mandates comprehensive CI +builder environment checks, and enforces centralized Python static analysis. +Furthermore, it outlines uncompromising standards for atomic commit metadata and +pragmatic testing state isolation to guarantee that every integration is fully +bisectable and verifiable. + +For incoming engineers, adherence to these mandates eliminates the friction of +stalled pipelines, unreviewable monolithic changes, and silent CI regressions. +Treat this guide as your primary roadmap for navigating the repository's strict +submission requirements, enabling seamless transitions from peer approval to +successfully integrated code. + +## Summary + +| Chapter Theme / Title | Scope & Objective | +| :------------------------------- | :---------------------------------------- | +| **Gerrit Submission and Labeling | Dictates strict access controls, review | +: Workflow** : enforcement protocols, and Gerrit : +: : labeling mechanisms required to advance : +: : changes through the CI pipeline, ensuring : +: : seamless transitions to automated : +: : integration via the Commit-Queue. : +| **CI Builder Environment and | Defines guidelines for ensuring build | +: Execution Integrity** : script resilience against missing : +: : dependencies and managing process : +: : execution contexts within LUCI and local : +: : testing environments to prevent silent : +: : builder failures. : +| **Python Code Formatting and | Governs the automated enforcement of | +: Linting** : Python style guidelines, mandating strict : +: : PEP-8 compliance, import sorting, and : +: : consistent string quoting to ensure : +: : codebase uniformity and prevent CI : +: : regressions. : +| **Commit Metadata and History | Establishes the structural composition | +: Standardization** : and metadata formatting of change lists : +: : (CLs) to ensure precise issue tracker : +: : integration, reliable CI/CD parsing, and : +: : an atomic, bisectable repository history. : +| **Testing Strategy and State | Outlines test implementation boundaries, | +: Isolation** : emphasizing pragmatic mocking limits to : +: : prevent false positives and detailing : +: : acceptable workflows for deferred test : +: : coverage while maintaining verification : +: : integrity. : + +-------------------------------------------------------------------------------- +-------------------------------------------------------------------------------- + +## Chapter: Gerrit Submission and Labeling Workflow + +**Context:** This domain dictates the strict access controls, review enforcement +protocols, and specific Gerrit labeling mechanisms required to advance changes +through the CI pipeline. Adherence ensures seamless transitions from peer +approval to automated integration via the Commit-Queue. + +### Summary + +| Rule ID | Principle / Constraint | Priority | Primary Symptom / | +: : : : Trap : +| :-------- | :------------------------------ | :------- | :----------------- | +| **T1-01** | Explicit Labeling for Gerrit | High | Leaving a change | +: : Automated Submission : : idle after : +: : : : addressing : +: : : : comments or : +: : : : receiving a : +: : : : reviewer's LGTM, : +: : : : expecting the : +: : : : reviewer to merge : +: : : : it. : +| **T1-02** | Automated Submission via | Medium | Requesting a | +: : Commit-Queue (CQ) : : manual push or : +: : : : direct submit from : +: : : : repository : +: : : : maintainers after : +: : : : receiving code : +: : : : review approval. : +| **T1-03** | Gerrit Trusted Contributor | Medium | Relying on a | +: : Review Enforcement Verification : : standard +2 vote : +: : : : from a non-trusted : +: : : : contributor to : +: : : : fulfill strict : +: : : : Review-Enforcement : +: : : : requirements. : +| **T1-04** | Mandatory Gerrit Labels for | High | Acknowledging an | +: : Automated Submission : : approval but : +: : : : failing to apply : +: : : : the appropriate : +: : : : Gerrit labels to : +: : : : initiate the merge : +: : : : pipeline. : +| **T1-05** | Gerrit Automated Submission | Medium | Leaving an | +: : Triggers : : approved patchset : +: : : : idle and waiting : +: : : : for maintainers to : +: : : : manually merge it. : +| **T1-06** | Active Reviewer Rerouting for | Medium | Waiting weeks or | +: : Stalled Changes : : months for an : +: : : : inactive or OOO : +: : : : reviewer to : +: : : : respond to a : +: : : : patchset update. : + +-------------------------------------------------------------------------------- + +### Rules + +#### T1-01: Explicit Labeling for Gerrit Automated Submission + +> **Rule:** Always apply `Verified+1` and `Commit-Queue+2` explicitly to trigger +> the final submission phase. Never assume a code approval automatically +> initiates the pipeline. +> +> **What:** Changes are not merged automatically upon receiving approval; +> contributors must explicitly set the `Verified+1` and `Commit-Queue+2` labels +> to trigger the final submission phase. +> +> **Applies To:** Gerrit review UI and change submission pipeline as defined in +> `CONTRIBUTING.md`. +> +> **Why:** Contributors often mistakenly assume an LGTM implies an immediate +> merge, leading to stalled changes. The project relies on explicitly triggering +> the Commit-Queue to finalize CI checks and perform the merge. Failing to +> adhere to this typically results in **Stalled Submission Pipeline**. + +**Trap 1: Leaving a change idle after addressing comments or receiving a +reviewer's LGTM, expecting the reviewer to merge it.** + +**Don't:** + +* Waiting indefinitely after reviewer posts 'LGTM'. + +**Do:** + +* Vote `Verified+1` and `Commit-Queue+2` manually to submit the change to the + automated queue. + +**Exceptions:** Contributors lacking trusted permissions must ping a repository +maintainer to apply the final `Commit-Queue+2` vote. + +-------------------------------------------------------------------------------- + +#### T1-02: Automated Submission via Commit-Queue (CQ) + +> **Rule:** Must utilize the Gerrit Commit-Queue (CQ) labeling system to merge +> code. Maintainers must never perform direct manual submissions. +> +> **What:** Merging code must be triggered via the Gerrit Commit-Queue (CQ) +> labeling system rather than relying on direct manual submission by +> maintainers. +> +> **Applies To:** Gerrit code review UI and CI/CD submission workflow. +> +> **Why:** Contributors would request maintainers to directly merge patches once +> approved, bypassing the automated commit-queue pipeline, which guarantees that +> final integration tests pass before pushing to the target branch. Failing to +> adhere to this typically results in **Bypassed CI / Direct Submit**. + +**Trap 1: Requesting a manual push or direct submit from repository maintainers +after receiving code review approval.** + +**Don't:** + +* Leaving a comment: "I believe everything is ready for integrating this. So + if either of you can submit it, it would be appreciated." + +**Do:** + +* Applying the `Commit-Queue+2` (CQ+2) label in Gerrit, which delegates + testing and the final merge operation to the automated bot. + +-------------------------------------------------------------------------------- + +#### T1-03: Gerrit Trusted Contributor Review Enforcement Verification + +> **Rule:** Verify review enforcement requirements are satisfied by contributors +> within the explicitly configured trusted group. Never cast misleading +2 votes +> if you lack valid trusted group privileges. +> +> **What:** Gerrit submission requirements may mandate specific approval levels +> (e.g., two trusted contributors). Votes from users with +2 access who are not +> in the designated 'trusted' group do not satisfy the 'Review-Enforcement' +> submit requirement. +> +> **Applies To:** Gerrit repository administration and code review voting +> workflows. +> +> **Why:** Non-trusted contributors with +2 rights were casting +2 votes on +> changes. These votes did not fulfill the 'Two trusted contributors' +> Review-Enforcement requirement, leading to stalled submissions and confusion +> regarding why the UI showed a +2 but blocked submission. Failing to adhere to +> this typically results in **Blocked Submission / Silent Requirement Failure**. + +**Trap 1: Relying on a standard +2 vote from a non-trusted contributor to +fulfill strict Review-Enforcement requirements.** + +**Don't:** + +* Leaving a +2 vote on a change as a non-trusted contributor, creating the + false appearance that the Review-Enforcement requirement has been partially + or fully met. + +**Do:** + +* Verifying the reviewer is in the explicitly configured trusted group for the + repository. If not, the reviewer should manually downgrade their invalid +2 + vote to a +1 to clearly indicate that their vote does not count toward the + enforcement threshold. + +**Exceptions:** Repositories where specific non-employee groups have been +explicitly added to the trusted administrators list. + +-------------------------------------------------------------------------------- + +#### T1-04: Mandatory Gerrit Labels for Automated Submission + +> **Rule:** Always apply `Verified+1` and `Commit-Queue+2` labels to initiate +> the CI merge process. Never leave an approved CL in a technically unlabeled +> state. +> +> **What:** A code change must receive explicit `Verified+1` and +> `Commit-Queue+2` labels by the author or reviewer to trigger the automated CI +> merge process. +> +> **Applies To:** Gerrit workflow / Merge execution phase. +> +> **Why:** Historically, leaving a Change List (CL) in an approved but unlabeled +> state causes the integration pipeline to stall indefinitely, requiring manual +> intervention or reviewer pinging to trigger the CI queue. Failing to adhere to +> this typically results in **Merge Pipeline Stall**. + +**Trap 1: Acknowledging an approval but failing to apply the appropriate Gerrit +labels to initiate the merge pipeline.** + +**Don't:** + +* Leaving the CL in an approved state and waiting for auto-submission without + applying the `Verified+1` or `Commit-Queue+2` labels. + +**Do:** + +* Explicitly applying `Verified+1` (and `Commit-Queue+2` if ready) once + reviewers have approved the logic, to instruct the automation to merge the + code. + +-------------------------------------------------------------------------------- + +#### T1-05: Gerrit Automated Submission Triggers + +> **Rule:** Must actively signal patch readiness to Gerrit systems using proper +> label thresholds. Avoid leaving patchsets idle assuming upstream maintainer +> action. +> +> **What:** A patchset requires specific label thresholds ('Verified+1' and +> 'Commit-Queue+2') to trigger automated submission in the Gerrit workflow. +> +> **Applies To:** Gerrit review UI and automated CI/CD submission process for +> the git-repo codebase. +> +> **Why:** Contributors frequently asked how to integrate changes after +> receiving an approval, leading to stalled patches because the automated +> pipeline was not explicitly triggered. Failing to adhere to this typically +> results in **Stalled Patch Integration**. + +**Trap 1: Leaving an approved patchset idle and waiting for maintainers to +manually merge it.** + +**Don't:** + +* Waiting indefinitely after receiving an 'LGTM' without setting workflow + labels. + +**Do:** + +* The patch author manually sets the 'Verified' flag (if locally tested) and + applies the 'Commit-Queue+2' vote to signal readiness for automated merge. + +-------------------------------------------------------------------------------- + +#### T1-06: Active Reviewer Rerouting for Stalled Changes + +> **Rule:** Actively reroute reviews stalled by unresponsive or out-of-office +> (OOO) primary reviewers. Must explicitly tag alternate maintainers and +> document the absence to prevent lifecycle stalls. +> +> **What:** If the primary reviewer is out-of-office (OOO) or unresponsive for +> an extended period, contributors must actively CC and reroute the review to +> another active maintainer. +> +> **Applies To:** Gerrit review cycle and reviewer assignment process. +> +> **Why:** Patchsets have historically stalled for over a month due to reviewers +> taking extended leave without actively delegating their review queues. Failing +> to adhere to this typically results in **Indefinite Review Stalls**. + +**Trap 1: Waiting weeks or months for an inactive or OOO reviewer to respond to +a patchset update.** + +**Don't:** + +* Leaving a review assigned strictly to an unresponsive reviewer without + notifying other maintainers or attempting to escalate. + +**Do:** + +* Tag a new reviewer with 'PTAL' (Please Take A Look) in the thread, + explicitly noting the original reviewer's absence, and confirm alignment + with the original author. + +-------------------------------------------------------------------------------- + +### Cross-Domain Dependencies + +* **Upstream:** T4 | Python Code Formatting and Linting - *Proper formatting + and static analysis are enforced before changes become eligible for final + Gerrit review and automated integration.* +* **Upstream:** T5 | Commit Metadata and History Standardization - *Accurate + commit messaging and isolated history must be validated by reviewers prior + to receiving approval labels.* +* **Downstream:** T3 | CI Builder Environment and Execution Integrity - + *Triggering the Commit-Queue directly invokes downstream LUCI environments + to guarantee execution integrity prior to branch merge.* + +## Chapter: CI Builder Environment and Execution Integrity + +**Context:** This section defines strict guidelines for ensuring the resilience +of build scripts against missing dependencies and managing process execution +contexts within LUCI and local testing environments. Adherence guarantees robust +verification across diverse operating systems and CI pipelines while preventing +silent builder failures. + +### Summary + +| Rule ID | Principle / Constraint | Priority | Primary Symptom / Trap | +| :-------- | :------------------------ | :------- | :------------------------ | +| **T3-01** | Verification Against | High | Running a standard local | +: : Breaking Change Build : : `make` without testing : +: : Configurations : : strict configurations or : +: : : : breaking-change flags. : +| **T3-02** | Windows Developer Mode | Medium | Attempting to run full | +: : Requirements for Tool : : local verification on a : +: : Verification : : standard Windows user : +: : : : account. : +| **T3-03** | Graceful Degradation for | Medium | Assuming all local | +: : Missing Builder Utilities : : developer utilities exist : +: : : : in the strict CI builder : +: : : : environment and : +: : : : unconditionally executing : +: : : : them. : +| **T3-04** | Contextual Diagnostic | High | Observing a generic CI | +: : Logging for LUCI CI : : failure without isolating : +: : Failures : : the specific process : +: : : : execution context or : +: : : : dependency resolution : +: : : : step. : + +-------------------------------------------------------------------------------- + +### Rules + +#### T3-01: Verification Against Breaking Change Build Configurations + +> **Rule:** Always explicitly test core build structure modifications with +> breaking changes enabled to ensure forward compatibility. +> +> **What:** When modifying core build structures, the build must be tested +> explicitly with breaking changes enabled to ensure forward compatibility and +> correct regeneration of generated files. +> +> **Applies To:** Local build environments and Makefile targets. +> +> **Why:** Changes might succeed in a standard default build but fail when +> breaking change toggles are activated, hiding underlying dependency or +> regeneration issues. Failing to adhere to this typically results in **Build +> Breakage / Stale Artifacts**. + +**Trap 1: Running a standard local `make` without testing strict configurations +or breaking-change flags.** + +**Don't:** + +```bash +make -j +``` + +**Do:** + +```bash +make -j WITH_BREAKING_CHANGES=1 +``` + +-------------------------------------------------------------------------------- + +#### T3-02: Windows Developer Mode Requirements for Tool Verification + +> **Rule:** Must execute local tool verification on Windows (gWindows) using an +> Administrator account to enable Developer Mode. +> +> **What:** Local verification of git-repo tooling on Windows (gWindows) +> explicitly requires the host environment to be running with Administrator +> privileges to enable Developer Mode. +> +> **Applies To:** Windows (gWindows) test environments verifying file system +> operations. +> +> **Why:** Without Developer Mode enabled (which necessitates Admin rights), +> features relying on advanced OS-level file system operations (like symlinks) +> cannot execute, permanently blocking full local test suite execution on +> standard accounts. Failing to adhere to this typically results in +> **Verification Blocked / OS Permission Error**. + +**Trap 1: Attempting to run full local verification on a standard Windows user +account.** + +**Don't:** + +* Executing the test suite from a non-elevated command prompt on Windows + without Developer Mode. + +**Do:** + +* Elevate to an Administrator account to enable Developer Mode before + executing the test suite on gWindows. + +-------------------------------------------------------------------------------- + +#### T3-03: Graceful Degradation for Missing Builder Utilities + +> **Rule:** Always implement auto-skip logic for optional utilities in build +> scripts rather than hard-failing when unavailable on the CI builder. +> +> **What:** Build scripts and test suites must implement auto-skip logic for +> optional, environment-specific utilities rather than hard-failing when the +> utility is unavailable on the CI builder. +> +> **Applies To:** CI Builder environment scripts and test suites, specifically +> testing external CLI utilities (e.g., `help2man`). +> +> **Why:** When a required utility was not pre-installed on the CI builder +> image, the build hard-failed. Adding auto-skip logic allows the CI pipeline to +> remain unblocked while still providing local testing benefits for developers +> who have the tool installed. Failing to adhere to this typically results in +> **Build Failure / Blocked CI**. + +**Trap 1: Assuming all local developer utilities exist in the strict CI builder +environment and unconditionally executing them.** + +**Don't:** + +```python +# BAD: Hard failure if utility is missing +subprocess.run(["help2man", "repo"], check=True) +``` + +**Do:** + +```python +# GOOD: Auto-skip test if utility is missing in the environment +if not shutil.which("help2man"): + self.skipTest("help2man not installed") +subprocess.run(["help2man", "repo"], check=True) +``` + +**Exceptions:** Core dependencies required for fundamental build steps cannot be +skipped and must be installed on the bot image. + +-------------------------------------------------------------------------------- + +#### T3-04: Contextual Diagnostic Logging for LUCI CI Failures + +> **Rule:** Must investigate CI builder failures by extracting and analyzing +> full execution context logs to isolate environmental roadblocks. +> +> **What:** CI builder failures must be investigated using full execution +> context logs (e.g., LUCI context, vpython3 resolution, and retcode outputs) to +> isolate environmental roadblocks. +> +> **Applies To:** LUCI builder execution environment, vpython3 resolution, and +> CI pipeline debugging. +> +> **Why:** CI commands failed with `retcode 1` due to external factors like +> specific URLs being flagged as suspect by internal security tools, breaking +> the build environment. Failing to adhere to this typically results in **Silent +> Builder Failure**. + +**Trap 1: Observing a generic CI failure without isolating the specific process +execution context or dependency resolution step.** + +**Don't:** + +* Restarting the CI pipeline blindly when a job fails with a generic retcode, + ignoring potential external network or security blockers. + +**Do:** + +* Extract the step-by-step LUCI context log, verify path resolution (e.g., + CIPD packages), and explicitly document external blockers like security + flags in the review. + +-------------------------------------------------------------------------------- + +### Cross-Domain Dependencies + +* **Upstream:** T6 | Testing Strategy and State Isolation - *Test + implementation dictates how missing builder utilities are mocked or + gracefully skipped during execution.* +* **Downstream:** T1 | Gerrit Submission and Labeling Workflow - *Automated + Verified+1 labels rely entirely on the stable, unblocked execution of CI + builder pipelines.* + +## Chapter: Python Code Formatting and Linting + +**Context:** This domain governs the automated enforcement of Python style +guidelines, mandating strict PEP-8 compliance, import sorting, and consistent +string quoting. All Python modifications must pass centralized static analysis +pipelines before integration to ensure codebase uniformity and prevent CI +regressions. + +### Summary + +| Rule ID | Principle / Constraint | Priority | Primary Symptom / Trap | +| :-------- | :----------------------- | :------- | :------------------------ | +| **T4-01** | Automated Flake8 | Medium | Relying purely on manual | +: : Post-Submit Verification : : code review or sporadic : +: : : : local linting without a : +: : : : continuous integration : +: : : : check. : +| **T4-02** | Mandatory Python | High | Using single quotes for | +: : Formatting and Import : : strings and appending new : +: : Sorting : : imports to the bottom of : +: : : : the import block without : +: : : : alphabetical or : +: : : : categorical sorting. : +| **T4-03** | Strict Python Import | High | Mixing local application | +: : Ordering : : imports with standard : +: : : : library imports, causing : +: : : : linting tools to fail the : +: : : : CQ job. : + +-------------------------------------------------------------------------------- + +### Rules + +#### T4-01: Automated Flake8 Post-Submit Verification + +> **Rule:** Always configure and maintain centralized CI workflows to +> automatically run static analysis and validate Python code styling +> post-submit. +> +> **What:** Static analysis and Python linting must be automated via a +> centralized CI pipeline (e.g., Flake8 post-submit workflows) to enforce +> consistent style and prevent basic errors. +> +> **Applies To:** All Python files in the git-repo codebase; specifically +> validated via `.github/workflows/flake8-postsubmit.yml`. +> +> **Why:** Relying strictly on manual code review to catch styling and linting +> violations is error-prone. Automation ensures a baseline of code quality on +> every code push without consuming human review cycles. Failing to adhere to +> this typically results in **Linting Regression / Style Violation**. + +**Trap 1: Relying purely on manual code review or sporadic local linting without +a continuous integration check.** + +**Don't:** + +* Committing Python code without an active CI linting workflow configuration. + +**Do:** + +* Maintain `.github/workflows/flake8-postsubmit.yml` to automatically run + flake8 on target branches. + +-------------------------------------------------------------------------------- + +#### T4-02: Mandatory Python Formatting and Import Sorting + +> **Rule:** Must format Python code to enforce double-quoted strings and +> alphabetically sorted import blocks to satisfy automated formatting checks. +> +> **What:** Python code modifications must pass automated style and linting +> checks ('Verify git-repo CL'), which strictly enforce string quote conventions +> (preferring double quotes), import block sorting, and PEP-8 style formatting. +> +> **Applies To:** All Python source files modified in the git-repo codebase. +> +> **Why:** Developers submitting patches with single-quoted strings or unsorted +> imports triggered automated CI failures in the `Verify git-repo CL` job, +> completely blocking code submission until formatting tools were executed +> locally. Failing to adhere to this typically results in **CI Pipeline +> Failure**. + +**Trap 1: Using single quotes for strings and appending new imports to the +bottom of the import block without alphabetical or categorical sorting.** + +**Don't:** + +```python +import sys +import os + +msg = 'This is an error' +``` + +**Do:** + +```python +import os +import sys + +msg = "This is an error" +``` + +-------------------------------------------------------------------------------- + +#### T4-03: Strict Python Import Ordering + +> **Rule:** Always segment and order Python imports strictly according to +> project standards (standard library, third-party, local) to prevent CQ +> pipeline failures. +> +> **What:** Python module imports must adhere strictly to the project's +> formatting rules (e.g., standard library, third-party, local module ordering) +> to pass automated Commit-Queue (CQ) checks. +> +> **Applies To:** Python source files. +> +> **Why:** Non-standard import blocks cause the automated CI/CQ linting pipeline +> to fail, completely blocking submission even if the core functional logic of +> the patch is flawless. Failing to adhere to this typically results in **CI +> Linting Failure**. + +**Trap 1: Mixing local application imports with standard library imports, +causing linting tools to fail the CQ job.** + +**Don't:** + +```python +import sys +import my_local_module +import os +``` + +**Do:** + +```python +import os +import sys + +import my_local_module +``` + +-------------------------------------------------------------------------------- + +### Cross-Domain Dependencies + +* **Upstream:** T3 | CI Builder Environment and Execution Integrity - + *Reliable CI builder environments must be available to execute the static + analysis and Python formatting verifications.* +* **Downstream:** T1 | Gerrit Submission and Labeling Workflow - *Formatting + and linting rules must be fully satisfied before automated mechanisms like + the Commit-Queue (CQ+2) will merge code into the repository.* + +## Chapter: Commit Metadata and History Standardization + +**Context:** This domain governs the structural composition and metadata +formatting of change lists (CLs) within the git-repo codebase. Strict adherence +ensures precise issue tracker integration, reliable CI/CD parsing, and atomic, +bisectable repository history. + +### Summary + +| Rule ID | Principle / Constraint | Priority | Primary Symptom / Trap | +| :-------- | :------------------------ | :------- | :------------------------ | +| **T5-01** | Strict Commit Message Bug | Medium | Providing free-text | +: : Tag Formatting : : descriptions, arbitrary : +: : : : prefixes, or non-standard : +: : : : bug references in the : +: : : : commit block. : +| **T5-02** | Atomic and Bisectable | High | Waiting for an entire | +: : Change Integration : : feature stack of multiple : +: : : : interdependent CLs to be : +: : : : approved before merging : +: : : : the base commits. : +| **T5-03** | Explicit Bug Tracker | Medium | Submitting a fix or | +: : Linking for Context : : revert without : +: : Restoration : : referencing the : +: : : : corresponding bug tracker : +: : : : issue detailing the : +: : : : specific regression or : +: : : : stack trace. : +| **T5-04** | Atomic Change List | Medium | Submitting a single large | +: : Decomposition : : CL that touches multiple : +: : : : isolated components or : +: : : : implements several : +: : : : distinct features : +: : : : simultaneously. : + +-------------------------------------------------------------------------------- + +### Rules + +#### T5-01: Strict Commit Message Bug Tag Formatting + +> **Rule:** Must use the exact `Bug: <number>` syntax in commit messages to +> properly link issue trackers. +> +> **What:** Commit messages must link directly to issue trackers using the +> explicit 'Bug: <number>' syntax to allow reliable parsing by CI/CD and history +> tracking systems. +> +> **Applies To:** Commit messages across all git-repo changes. +> +> **Why:** Improperly formatted bug tags fail to link with the external issue +> tracker, severing historical context and breaking automated post-submit +> tracking workflows. Failing to adhere to this typically results in **Broken +> Traceability / Pre-submit Failure**. + +**Trap 1: Providing free-text descriptions, arbitrary prefixes, or non-standard +bug references in the commit block.** + +**Don't:** + +```text +Fixes bug 486536908 +Closes issue 486536908 +``` + +**Do:** + +```text +Bug: 486536908 +``` + +-------------------------------------------------------------------------------- + +#### T5-02: Atomic and Bisectable Change Integration + +> **Rule:** Always submit code incrementally as isolated, functional units +> rather than hoarding monolithic stacks. +> +> **What:** Code changes must be submitted incrementally as isolated, functional +> units rather than waiting to merge a massive interdependent stack all at once. +> +> **Applies To:** Git commit history, PR structuring, and stack-based code +> integration. +> +> **Why:** Contributors accustomed to integrating full monolithic stacks at once +> held off on landing initial, stable changes. This practice hinders the ability +> to isolate regressions via `git bisect` and prevents foundational code from +> "baking" in production. Failing to adhere to this typically results in +> **Bisection Breakage / Monolithic Rollbacks**. + +**Trap 1: Waiting for an entire feature stack of multiple interdependent CLs to +be approved before merging the base commits.** + +**Don't:** + +* Holding all changes in a stack locally or in code review until the final + feature patch is approved, then landing 10+ patches simultaneously. + +**Do:** + +* Landing initial, independent CLs one-by-one as soon as they are approved. + Ensuring each commit is independently usable and does not break the build. + +-------------------------------------------------------------------------------- + +#### T5-03: Explicit Bug Tracker Linking for Context Restoration + +> **Rule:** Must include a direct URL to the relevant bug tracker issue +> documenting the failure traceback when submitting a regression fix or revert. +> +> **What:** When submitting a change (especially a revert or bug fix) addressing +> a specific runtime regression, the commit metadata or patchset-level comments +> must include a direct link to the bug tracker issue documenting the failure +> traceback. +> +> **Applies To:** Commit messages and patchset documentation during code +> reviews, particularly for reverts. +> +> **Why:** A previous commit caused a runtime regression (e.g., an +> AttributeError related to a missing object attribute). Without linking the +> specific issue containing the traceback, reviewers lacked the necessary +> context to justify restoring the previous codebase state. Failing to adhere to +> this typically results in **Undocumented Regression / Context Loss**. + +**Trap 1: Submitting a fix or revert without referencing the corresponding bug +tracker issue detailing the specific regression or stack trace.** + +**Don't:** + +* Reverting a change with a vague description like "Fixing previous breakage" + or "Reverting due to pipeline failure" without providing the traceback + source. + +**Do:** + +* Linking the specific issue tracker URL containing the exact failure mode. + Example: "for more context, see + https://g-issues.gerritcodereview.com/issues/[ISSUE_ID]#comment4" + +-------------------------------------------------------------------------------- + +#### T5-04: Atomic Change List Decomposition + +> **Rule:** Never submit large, monolithic change lists; always decompose them +> into logically independent patchsets. +> +> **What:** Large, monolithic change lists (CLs) must be broken down into +> smaller, logically independent patchsets to ensure accurate review and +> historical bisectability. +> +> **Applies To:** Version control history and code review scoping. +> +> **Why:** Massive CLs heavily increase reviewer cognitive load, making thorough +> reviews impossible and complicating future `git bisect` operations when +> tracking down the origin of a regression. Failing to adhere to this typically +> results in **Unreviewable Monolithic Change**. + +**Trap 1: Submitting a single large CL that touches multiple isolated components +or implements several distinct features simultaneously.** + +**Don't:** + +* A single CL containing sweeping refactoring, new feature implementation, and + unrelated bug fixes. + +**Do:** + +* Breaking the monolithic change into smaller, logically dependent or + independent CLs where each addresses one specific piece of the feature or + refactor. + +-------------------------------------------------------------------------------- + +### Cross-Domain Dependencies + +* **Downstream:** T1 | Gerrit Submission and Labeling Workflow - *Gerrit and + CI pipelines strictly rely on standardized commit metadata to link tracking + issues and depend on atomic patchsets to execute automated review and + verification correctly.* + +## Chapter: Testing Strategy and State Isolation + +**Context:** This chapter governs test implementation boundaries, emphasizing +pragmatic mocking limits to prevent false positives and detailing acceptable +workflows for deferred test coverage. Strict adherence ensures robust state +isolation and maintains development velocity without compromising verification +integrity. + +### Summary + +| Rule ID | Principle / Constraint | Priority | Primary Symptom / Trap | +| :-------- | :----------------------- | :------- | :------------------------- | +| **T6-01** | Pragmatic Mocking | Medium | Mocking the entire core | +: : Boundaries in Unit Tests : : state or framework : +: : : : dependencies just to force : +: : : : a unit test for a highly : +: : : : integrated function. : +| **T6-02** | Deferred Test | Medium | Submitting functional code | +: : Implementation via : : without matching test : +: : Follow-up : : coverage and stalling the : +: : : : merge while complex tests : +: : : : are written. : + +-------------------------------------------------------------------------------- + +### Rules + +#### T6-01: Pragmatic Mocking Boundaries in Unit Tests + +> **Rule:** Always restrict unit tests to isolated methods and avoid aggressive +> mocking of core functionality to prevent brittle, false-positive verification. +> +> **What:** Do not aggressively mock core functionality in unit tests; restrict +> unit tests to isolated methods to avoid creating brittle tests based on false +> assumptions when an integration framework is unavailable. +> +> **Applies To:** Test suite implementation (Unit vs. Integration testing +> boundaries). +> +> **Why:** Over-mocking complex systems in unit tests leads to scenarios where +> tests pass but the core integration fails in production because the unit test +> mocks assumed incorrect behavior about the underlying environment. Failing to +> adhere to this typically results in **False Positive Test Passage**. + +**Trap 1: Mocking the entire core state or framework dependencies just to force +a unit test for a highly integrated function.** + +**Don't:** + +* Mocking file systems, external processes, and global state heavily to test a + core workflow orchestrator in a unit test suite. + +**Do:** + +* Limiting unit tests strictly to isolated utility methods (e.g., adding + promisor files) and explicitly documenting testing gaps that require + integration test frameworks. + +**Exceptions:** Isolated helper methods or purely functional data +transformations should be fully unit tested with appropriate mocked inputs. + +-------------------------------------------------------------------------------- + +#### T6-02: Deferred Test Implementation via Follow-up + +> **Rule:** Never stall critical feature merges indefinitely for test +> implementation if maintainers authorize formalized, immediate follow-up test +> coverage. +> +> **What:** New logic requires automated tests; however, reviewers may permit +> test coverage to be implemented in a subsequent follow-up CL to maintain +> development velocity. +> +> **Applies To:** Feature development, regression testing, and code review +> criteria. +> +> **Why:** Reviewers identified a lack of test coverage for new functionality +> but opted not to block the immediate patchset, instead formalizing the test +> requirement as a near-term follow-up task. Failing to adhere to this typically +> results in **Missing Test Coverage**. + +**Trap 1: Submitting functional code without matching test coverage and stalling +the merge while complex tests are written.** + +**Don't:** + +* Blocking a necessary feature indefinitely due to missing unit tests when a + follow-up CL is viable and acceptable to maintainers. + +**Do:** + +* Approve the feature with an explicit, documented 'TODO' for a follow-up CL + dedicated strictly to adding the corresponding automated tests. + +**Exceptions:** Critical path features or security fixes where a lack of +immediate coverage introduces an unacceptable regression risk. + +-------------------------------------------------------------------------------- + +### Cross-Domain Dependencies + +* **Upstream:** T1 | Gerrit Submission and Labeling Workflow - *Reviewer + approval mechanisms and label enforcement dictate when a feature can merge + while deferring tests to a follow-up CL.* +* **Downstream:** T3 | CI Builder Environment and Execution Integrity - + *Pragmatically bounded unit and integration tests ensure reliable CI + pipeline execution without false-positive success markers.*
diff --git a/agents/skills/core_internals/SKILL.md b/agents/skills/core_internals/SKILL.md new file mode 100644 index 0000000..2579646 --- /dev/null +++ b/agents/skills/core_internals/SKILL.md
@@ -0,0 +1,6212 @@ +--- +name: core-internals +description: Provides guidance and best practices on concurrent synchronization, IPC, filesystem atomicity, git subprocess wrapping, XML manifests, and CLI argument parsing for git-repo. +--- + +# Git Repo Core Internals Engineering Guide + +## Executive Summary + +Welcome to the authoritative engineering guide for Git Repository Management +within the git-repo codebase. This repository of folk knowledge exists to +safeguard the intricate orchestration of our multi-repository synchronization +toolchain. Historically, complex operations such as parallelized network +fetches, layered manifest overrides, and interactive terminal rendering have +been vulnerable to subtle concurrency races, deadlocks, and fragile system +states. This guide captures these critical failure modes—ranging from IPC +serialization bottlenecks and filesystem lock contentions to non-hermetic test +pollution—and establishes rigid engineering constraints to prevent their +regression. + +To maintain system stability, this guide enforces strict architectural +boundaries across the tooling ecosystem. It mandates stateless execution for +multiprocessing pools, guaranteed atomicity for worktree layout modifications, +and deterministic error translation for all standard Git subprocesses. +Furthermore, it defines the standard operating procedures for canonical manifest +object deduplication, the modernization of our hermetic testing frameworks, and +the delivery of consistent, machine-readable CLI interfaces. By adhering to +these paradigms, incoming engineers will ensure the reliability, performance, +and extensibility of git-repo's core repository management infrastructure. + +## Summary + +| Chapter Theme / Title | Scope & Objective | +| :----------------------------------- | :------------------------------------ | +| **Concurrent Synchronization & IPC** | This domain governs the stable | +: : orchestration of parallel network : +: : fetches, local checkouts, and : +: : interleaved subprocess routines. It : +: : strictly enforces safe : +: : multiprocessing IPC, deterministic : +: : Git locking via exponential backoff, : +: : and state synchronization across : +: : concurrent pool workers. : +| **Filesystem Atomicity & Worktree | This domain governs the deterministic | +: Layout** : creation, migration, and cleanup of : +: : internal repository structures and : +: : Git worktrees. It relies on ephemeral : +: : temporary directories, atomic rename : +: : operations, and robust error recovery : +: : to prevent corrupted states during : +: : unexpected interruptions. : +| **Subprocess Git Integration & Error | This chapter defines the constraints | +: Translation** : for wrapping, executing, and : +: : translating standard Git subprocesses : +: : within the Repo tooling ecosystem. It : +: : mandates the use of centralized : +: : command abstractions, strict version : +: : gating, and deterministic stream : +: : handling to guarantee reliable : +: : repository state management and : +: : actionable error reporting. : +| **Manifest Object Model & | This chapter governs the parsing, | +: Deduplication** : validation, and canonicalization of : +: : XML manifest components. It strictly : +: : enforces semantic immutability via : +: : NamedTuple implementations, defensive : +: : copying for hierarchical override : +: : scoping, and deterministic : +: : JSON-backed file tracking across the : +: : subsystem. : +| **Hermetic Testing & Test | This domain governs the migration of | +: Modernization** : legacy unittest suites to modern : +: : pytest functional paradigms and the : +: : establishment of hermetic session : +: : fixtures. It enforces strict : +: : environment isolation to prevent : +: : global state pollution (e.g., : +: : developer .gitconfig bleeding) while : +: : safely intercepting standard streams : +: : and filesystem paths. : +| **CLI Argument Parsing & UX | This chapter governs the lifecycle, | +: Consistency** : validation, and execution of : +: : command-line arguments, enforcing : +: : strict standardization for : +: : machine-readable serialization, : +: : unified logging, and deterministic, : +: : thread-safe terminal interactions. : +| **Repo Hooks Framework** | The Repo Hooks Framework governs the | +: : execution, parameter validation, and : +: : lifecycle management of user-defined : +: : scripts within the repository : +: : ecosystem. It ensures seamless : +: : integration of extensions like : +: : post-sync or pre-upload while : +: : strictly isolating their execution : +: : failures from core operational : +: : workflows. : + +-------------------------------------------------------------------------------- +-------------------------------------------------------------------------------- + +## Chapter: Concurrent Synchronization & IPC + +**Context:** This domain governs the stable orchestration of parallel network +fetches, local checkouts, and interleaved subprocess routines. It strictly +enforces safe multiprocessing IPC, deterministic Git locking via exponential +backoff, and state synchronization across concurrent pool workers. + +### Summary + +| Rule ID | Principle / Constraint | Priority | Primary Symptom / Trap | +| :-------- | :------------------------ | :------- | :------------------------ | +| **T1-01** | Stateless Class Methods | High | Passing bound instance | +: : for Parallel Pool Workers : : methods to a parallel : +: : : : executor, dragging : +: : : : unnecessary state into : +: : : : the multiprocessing : +: : : : serialization pipeline. : +| **T1-02** | Buffered Serialization of | High | Executing terminal print | +: : Concurrent Standard : : calls directly from : +: : Output : : within a parallelized : +: : : : worker routine. : +| **T1-03** | Interleaved Sync Path | High | Dispatching parallel | +: : Validation : : checkout jobs across all : +: : : : project variants without : +: : : : verifying layout types. : +| **T1-04** | Guaranteed | Critical | Placing save operations | +: : Synchronization State : : at the very end of an : +: : Persistence : : execution path without : +: : : : exception guards. : +| **T1-05** | Jittered Exponential | Critical | Firing `git submodule | +: : Backoff for Git : : init` concurrently : +: : Configuration Locks : : without retry logic for : +: : : : `config.lock` failures. : +| **T1-06** | Exponential Backoff for | High | Failing a git command | +: : Concurrent Git Config : : immediately without : +: : Locks : : evaluating standard : +: : : : streams for transient : +: : : : lock errors. : +| **T1-07** | Hard Termination on | Critical | Logging an error state | +: : Interleaved Sync Stalls : : inside a `while` loop but : +: : : : allowing the next loop : +: : : : iteration to execute. : +| **T1-08** | Resource Management via | High | Initializing proxy | +: : Context Handlers for IPC : : connections or : +: : : : multiprocessing managers : +: : : : as raw variable : +: : : : assignments without a : +: : : : guaranteed teardown : +: : : : phase. : +| **T1-09** | Dual-Channel Error | High | Iterating over aggregated | +: : Handling in Parallel : : data lists to decide if a : +: : Processing : : parallel orchestration : +: : : : should abort. : +| **T1-10** | Minimum Process Pool Job | Critical | Directly using | +: : Count Safeguard : : `min(target, len(items))` : +: : : : to determine pool size, : +: : : : which breaks if the item : +: : : : list is unexpectedly : +: : : : empty. : +| **T1-11** | Safeguarding Object State | Critical | Truncating object | +: : Across Parallel Execution : : payloads to scalar : +: : Boundaries : : indices during IPC : +: : : : context setup without : +: : : : updating the receiver : +: : : : logic to rehydrate the : +: : : : objects. : +| **T1-12** | Explicit Context | High | Assuming child workers | +: : Initialization for : : inherit updated class : +: : Multiprocessing Pools : : variables inherently : +: : : : without explicit : +: : : : initialization. : +| **T1-13** | Dynamic Task Chunk Sizing | Medium | Passing a hardcoded batch | +: : in Parallel Execution : : integer to the : +: : : : `chunksize` parameter. : +| **T1-14** | Deferred Worktree | High | Returning early from the | +: : Operations in Sync Local : : sync phase without : +: : Half : : applying required file : +: : : : operations. : + +-------------------------------------------------------------------------------- + +### Rules + +#### T1-01: Stateless Class Methods for Parallel Pool Workers + +> **Rule:** Always implement multiprocessing pool targets as stateless class +> methods or standalone functions to bypass process serialization constraints. +> +> **What:** Worker execution targets within multiprocessing pools must be +> constructed as fully decoupled class methods or standalone functions to bypass +> process serialization constraints. +> +> **Applies To:** Multiprocessing process pools, concurrent task scheduling, and +> `ExecuteInParallel` implementations. +> +> **Why:** Command execution objects were heavily bound to system state. +> Attempting to pass instance methods (`self.method`) to a multiprocessing pool +> frequently caused `PicklingError` crashes, as the underlying Python +> serialization mechanism cannot cleanly isolate bound object graphs. Failing to +> adhere to this typically results in **IPC Serialization Error**. + +**Trap 1: Passing bound instance methods to a parallel executor, dragging +unnecessary state into the multiprocessing serialization pipeline.** + +**Don't:** + +```python +class InfoCommand: + def _worker_logic(self, project): + pass + + def run(self): + self.ExecuteInParallel(jobs, self._worker_logic, projects) +``` + +**Do:** + +```python +class InfoCommand: + @classmethod + def _worker_logic(cls, project_idx): + project = cls.get_parallel_context()["projects"][project_idx] + pass + + def run(self): + self.ExecuteInParallel(jobs, self._worker_logic, range(len(projects))) +``` + +**Exceptions:** Threading-based execution models where memory is shared and +pickling is not strictly enforced. + +-------------------------------------------------------------------------------- + +#### T1-02: Buffered Serialization of Concurrent Standard Output + +> **Rule:** Must capture standard output generated by concurrently executing +> tasks into isolated memory buffers for sequential display in the parent +> process. +> +> **What:** Data emitted by concurrently executing tasks must be captured into +> isolated memory buffers and returned to the parent process for sequential +> display. +> +> **Applies To:** Parallel process execution layers generating human-readable +> CLI output. +> +> **Why:** When command execution was parallelized, worker processes wrote +> directly to standard output. This created severe race conditions resulting in +> garbled, interleaved text output on the user's terminal. Failing to adhere to +> this typically results in **Interleaved Terminal Output**. + +**Trap 1: Executing terminal print calls directly from within a parallelized +worker routine.** + +**Don't:** + +```python +@classmethod +def _DiffHelper(cls, project): + print(f"Project: {project.name}") + print(f"Revision: {project.rev}") +``` + +**Do:** + +```python +@classmethod +def _DiffHelper(cls, project): + buf = io.StringIO() + buf.write(f"Project: {project.name}\n") + buf.write(f"Revision: {project.rev}\n") + return buf.getvalue() + +# In main process: +for output in results: + print(output, end="") +``` + +-------------------------------------------------------------------------------- + +#### T1-03: Interleaved Sync Path Validation + +> **Rule:** Always validate the presence of a worktree when executing +> interleaved sync processes to prevent layout evaluation crashes. +> +> **What:** When executing interleaved sync processes (parallelized network +> fetches and checkouts), the operation must explicitly validate the presence of +> a worktree, as not all Git repository types (e.g., mirrors) maintain local +> file checkouts. +> +> **Applies To:** Concurrency logic within `sync.py`. +> +> **Why:** Changing the default mode of `repo sync` to interleaved parallelized +> checkout tasks indiscriminately. This immediately broke AOSP mirror syncing +> because mirrors lack local checkout paths, resulting in a `TypeError` when +> evaluating `NoneType` paths. Failing to adhere to this typically results in +> **TypeError / Sync Failure**. + +**Trap 1: Dispatching parallel checkout jobs across all project variants without +verifying layout types.** + +**Don't:** + +* Assuming `project.worktree` always contains an `os.PathLike` object during + interleaved syncing. + +**Do:** + +* Adding guard clauses to verify `project.worktree is not None` and handling + `--mirror` modes explicitly before adding checkout tasks to the thread pool. + +-------------------------------------------------------------------------------- + +#### T1-04: Guaranteed Synchronization State Persistence + +> **Rule:** Must execute synchronization telemetry saving operations within +> `try...finally` blocks to guarantee data retention across execution +> interrupts. +> +> **What:** Core sync operations must wrap network and filesystem operations in +> `try...finally` blocks, ensuring that synchronization metadata (`_fetch_times` +> and `_local_sync_state`) is persisted even if the sync operation fails or is +> interrupted. +> +> **Applies To:** `subcmds/sync.py`, particularly the `_Fetch` logic and +> multiprocessing worker loops. +> +> **Why:** If a synchronization process encountered an error (like a fetch +> failure), the operation exited early, dropping valuable telemetry and +> optimization data (fetch times) for projects that successfully synced prior to +> the crash. Failing to adhere to this typically results in **Telemetry Loss / +> Sync State Inconsistency**. + +**Trap 1: Placing save operations at the very end of an execution path without +exception guards.** + +**Don't:** + +```python +result = self._Fetch(to_fetch, opt, err_event) +if not result.success: + raise SyncError("failed") + +# BAD: Never reached if an error is raised +self._fetch_times.Save() +``` + +**Do:** + +```python +try: + result = self._Fetch(to_fetch, opt, err_event) + if not result.success: + raise SyncError("failed") +finally: + # GOOD: Always saves telemetry state + self._fetch_times.Save() +``` + +-------------------------------------------------------------------------------- + +#### T1-05: Jittered Exponential Backoff for Git Configuration Locks + +> **Rule:** Always wrap concurrent Git mutations to shared configurations in an +> exponential backoff routine with jitter to mitigate transient filesystem +> locks. +> +> **What:** When initializing submodules concurrently (e.g., `git submodule +> init`), the system must employ an exponential backoff mechanism with +> randomized jitter to handle transient filesystem lock errors on `.git/config`. +> +> **Applies To:** Submodule initialization and any highly parallel Git +> subprocesses mutating shared configuration state. +> +> **Why:** Running `repo sync -j<N>` caused multiple child processes to +> simultaneously attempt `git submodule init`, which led to lock contention on +> the parent project's config file and caused the entire sync operation to fail. +> Failing to adhere to this typically results in **File Lock Contention / Sync +> Failure**. + +**Trap 1: Firing `git submodule init` concurrently without retry logic for +`config.lock` failures.** + +**Don't:** + +```python +# BAD: Fails immediately if .git/config.lock exists +subprocess.run(["git", "submodule", "init", "--", path], check=True) +``` + +**Do:** + +```python +# GOOD: Catch lock errors and retry with jitter +for attempt in range(MAX_RETRIES): + p = subprocess.run(["git", "submodule", "init", "--", path], stderr=subprocess.PIPE) + if p.returncode == 0: + break + if "could not lock config file" in p.stderr: + time.sleep(base_delay * (2 ** attempt) + random.uniform(0, jitter)) +``` + +**Exceptions:** Non-lock related Git errors should still fail immediately +without retrying. + +-------------------------------------------------------------------------------- + +#### T1-06: Exponential Backoff for Concurrent Git Config Locks + +> **Rule:** Must analyze captured Git error streams to detect config lock +> contention and orchestrate retries via structured logging instead of immediate +> process failure. +> +> **What:** Concurrent git operations modifying `.git/config` (such as submodule +> initialization) must utilize an exponential retry mechanism with jitter to +> handle transient filesystem lock contention. Standard output and error streams +> must be captured and parsed to detect these locks. +> +> **Applies To:** project.py, specifically concurrent `git submodule init` +> operations or any parallel processes altering local repository configuration. +> +> **Why:** When synchronizing with high job values (-j), multiple parallel +> processes modifying `.git/config` triggered race conditions, throwing 'could +> not lock config file' errors and aborting the sync. Failing to adhere to this +> typically results in **Transient Sync Failure / Lock Contention**. + +**Trap 1: Failing a git command immediately without evaluating standard streams +for transient lock errors.** + +**Don't:** + +```python +# BAD: Fails immediately on lock contention +if GitCommand(self, cmd).Wait() != 0: + raise GitError(f"{self.name} submodule init failed") +``` + +**Do:** + +```python +# GOOD: Captures stdout/stderr and retries on 'lock' matches +git_cmd = GitCommand(None, cmd, capture_stdout=True, capture_stderr=True) +if git_cmd.Wait() != 0: + error = git_cmd.stderr or git_cmd.stdout + if "lock" in error: + # apply exponential backoff + jitter logic + else: + git_cmd.VerifyCommand() # Propagate real error +``` + +**Trap 2: Using raw `print` statements to log retry attempts, which breaks +structured output formats.** + +**Don't:** + +```python +# BAD: Unstructured print to stdout +print(f"Attempt {attempt+1}: git {' '.join(cmd)} failed. Sleeping...") +``` + +**Do:** + +```python +# GOOD: Forwarded to centralized logger +logger.warning("Attempt %d/%d: git %s failed. Error: %s. Sleeping %.2fs before retrying.", attempt+1, max_retries, cmd, error, delay) +``` + +**Exceptions:** Non-lock related Git errors should break the retry loop and be +propagated immediately via `git_cmd.VerifyCommand()`. + +-------------------------------------------------------------------------------- + +#### T1-07: Hard Termination on Interleaved Sync Stalls + +> **Rule:** Must trigger a definitive execution break and flag error signals +> whenever an unresolvable stall is detected in an interleaved control loop. +> +> **What:** When an unresolvable stall is detected within an interleaved or +> parallel execution loop, the system must trigger a definitive state break +> (e.g., setting global events and breaking the loop) to prevent infinite +> deadlocks. +> +> **Applies To:** subcmds/sync.py, specifically `_SyncInterleaved` or any +> looping logic monitoring pending sets. +> +> **Why:** A bug existed where a stall was logged but the loop terminator +> (`break`) and error trigger (`err_event.set()`) were accidentally omitted. +> This allowed the sync routine to lock up infinitely when interdependent +> projects failed to checkout. Failing to adhere to this typically results in +> **Infinite Loop / Deadlock**. + +**Trap 1: Logging an error state inside a `while` loop but allowing the next +loop iteration to execute.** + +**Don't:** + +```python +# BAD: Omitted termination control flow +if prev_pending == pending_relpaths: + logger.error("Stall detected") +prev_pending = pending_relpaths +``` + +**Do:** + +```python +# GOOD: Set event signals and immediately break +if prev_pending == pending_relpaths: + logger.error("Stall detected") + err_event.set() + break +``` + +-------------------------------------------------------------------------------- + +#### T1-08: Resource Management via Context Handlers for IPC + +> **Rule:** Always initialize multiprocessing components and background proxies +> within `with` statement context blocks to assure teardown semantics. +> +> **What:** Multiprocessing managers and SSH proxies initialized for parallel +> workflows must be wrapped in `with` block context managers to guarantee +> cleanup of background processes and multiplexing sockets. +> +> **Applies To:** Concurrent network operations and parallelized repository +> synchronization (`subcmds/sync.py`). +> +> **Why:** When adding an interleaved fetch/checkout feature, parallel +> operations required shared synchronization dictionaries and SSH connection +> multiplexing. Without context managers, exceptions or interrupts (e.g., +> KeyboardInterrupt) would leave orphaned background processes and abandoned +> sockets. Failing to adhere to this typically results in **Resource Leaks / +> Orphaned Sockets**. + +**Trap 1: Initializing proxy connections or multiprocessing managers as raw +variable assignments without a guaranteed teardown phase.** + +**Don't:** + +```python +# BAD: No guaranteed cleanup on exception +manager = multiprocessing.Manager() +ssh_proxy = ssh.ProxyManager(manager) +process_tasks(ssh_proxy) +``` + +**Do:** + +```python +# GOOD: Context managers assure RAII semantics +with multiprocessing.Manager() as manager: + with ssh.ProxyManager(manager) as ssh_proxy: + process_tasks(ssh_proxy) +``` + +-------------------------------------------------------------------------------- + +#### T1-09: Dual-Channel Error Handling in Parallel Processing + +> **Rule:** Must decouple fast-fail signaling events from aggregated batch data +> processing logic to ensure immediate worker termination across processes. +> +> **What:** In concurrent execution environments, inter-process signaling (for +> immediate control flow) must be strictly separated from data aggregation (for +> end-of-batch error reporting). +> +> **Applies To:** Multi-process synchronization pools (`_SyncInterleaved`, +> `_SyncPhased`) and worker callback processors. +> +> **Why:** Reviewers questioned why a separate event flag was used when an array +> of errors was already being populated. Using only an aggregated list prevented +> workers from rapidly terminating each other via fail-fast mechanisms. Failing +> to adhere to this typically results in **Delayed Worker Termination**. + +**Trap 1: Iterating over aggregated data lists to decide if a parallel +orchestration should abort.** + +**Don't:** + +```python +# BAD: Waiting to process the whole batch before aborting +errors.append(result.error) +if errors and opt.fail_fast: + pool.close() +``` + +**Do:** + +```python +# GOOD: Using an IPC Event for immediate cross-process signaling +err_event.set() +errors.append(result.error) +if not ret and opt.fail_fast: + pool.close() +``` + +-------------------------------------------------------------------------------- + +#### T1-10: Minimum Process Pool Job Count Safeguard + +> **Rule:** Must bind dynamically computed multiprocessing job allocations to a +> minimum floor of 1 to prevent initialization crashes. +> +> **What:** When determining the number of worker processes for parallel +> execution based on a variable target length (e.g., an array of projects), the +> resulting worker count must be explicitly bound to a minimum of 1. +> +> **Applies To:** Network fetch logic and multiprocessing pool initializations +> (e.g., `_Fetch` in `sync.py`). +> +> **Why:** A regression occurred where passing an empty project list evaluated +> the worker count to 0. This crashed the multiprocessing pool initialization +> with a ValueError. Returning early was rejected as a fix because secondary +> side-effects (like state saves) within the function still needed to execute. +> Failing to adhere to this typically results in **ValueError / Process Pool +> Crash**. + +**Trap 1: Directly using `min(target, len(items))` to determine pool size, which +breaks if the item list is unexpectedly empty.** + +**Don't:** + +```python +jobs = min(opt.jobs_network, len(projects_list)) +``` + +**Do:** + +```python +jobs = max(1, min(opt.jobs_network, len(projects_list))) +``` + +-------------------------------------------------------------------------------- + +#### T1-11: Safeguarding Object State Across Parallel Execution Boundaries + +> **Rule:** Always transmit deeply serialized objects directly across parallel +> boundaries, or strictly reconstruct object relationships within the downstream +> worker. +> +> **What:** When offloading command execution to parallel processes, workers +> must receive properly serialized complex object data, not dissociated +> identifiers, unless the downstream API explicitly reconstructs the object +> state. +> +> **Applies To:** Parallel execution contexts (`ParallelContext`, +> `ExecuteInParallel`) within multi-project commands like `repo forall` and +> `repo upload`. +> +> **Why:** A refactoring attempt to optimize IPC by passing integer project +> indices instead of full project objects to parallel workers caused critical +> breakage. Worker processes subsequently attempted to access attributes (like +> `.manifest`) on integer types, leading to unhandled exceptions. Failing to +> adhere to this typically results in **AttributeError / Early Exit +> Regression**. + +**Trap 1: Truncating object payloads to scalar indices during IPC context setup +without updating the receiver logic to rehydrate the objects.** + +**Don't:** + +```python +# Worker receives an integer index instead of a Project +manifests = { + project.manifest.topdir: project.manifest +} +``` + +**Do:** + +```python +# Worker receives fully serialized Project objects (or properly reconstructs them) +manifests = { + project.manifest.topdir: project.manifest +} +``` + +-------------------------------------------------------------------------------- + +#### T1-12: Explicit Context Initialization for Multiprocessing Pools + +> **Rule:** Must dictate shared states through an explicit `initializer` +> argument injected into the multiprocessing pool rather than implicitly relying +> on execution fork behavior. +> +> **What:** Shared parallel context must be managed using an explicit context +> manager and passed via `initializer` and `initargs` in `multiprocessing.Pool`, +> rather than relying on unvalidated class attributes or fork memory semantics. +> +> **Applies To:** Any command or module utilizing `multiprocessing.Pool` (e.g., +> `ExecuteInParallel`). +> +> **Why:** Relying on `fork` memory semantics for sharing context works on some +> Linux systems but fails deterministically on environments (like macOS/Windows) +> where memory is not automatically shared or when the multiprocessing start +> method defaults to `spawn`. Failing to adhere to this typically results in +> **State Leakage / Uninitialized Context**. + +**Trap 1: Assuming child workers inherit updated class variables inherently +without explicit initialization.** + +**Don't:** + +```python +# BAD: Implicit state sharing +cls.parallel_context = data +with multiprocessing.Pool(jobs) as pool: + pool.imap_unordered(func, inputs) +``` + +**Do:** + +```python +# GOOD: Explicit initializer passed to workers +with multiprocessing.Pool( + jobs, + initializer=cls._SetParallelContext, + initargs=(cls._parallel_context,) +) as pool: + pool.imap_unordered(func, inputs) +``` + +**Trap 2: A context manager directly yielding an internal dictionary, allowing +unvalidated and lingering usage.** + +**Don't:** + +```python +# BAD: Direct yield of state without lifecycle validation +@contextlib.contextmanager +def ParallelContext(cls): + yield cls._parallel_context +``` + +**Do:** + +```python +# GOOD: Lifecycle management and defensive assertions +@contextlib.contextmanager +def ParallelContext(cls): + assert cls._parallel_context is None + cls._parallel_context = {} + try: + yield + finally: + cls._parallel_context = None +``` + +-------------------------------------------------------------------------------- + +#### T1-13: Dynamic Task Chunk Sizing in Parallel Execution + +> **Rule:** Always formulate the `chunksize` of mapping workers dynamically +> based on the dataset to job ratio to avoid workload serialization blocks. +> +> **What:** When utilizing `multiprocessing.Pool.imap_unordered`, the +> `chunksize` must be dynamically calculated based on the ratio of inputs to +> worker jobs, rather than statically hardcoded. +> +> **Applies To:** Parallel task dispatch mechanisms (`ExecuteInParallel`, +> synchronization loops). +> +> **Why:** Using a statically hardcoded chunk size (e.g., `WORKER_BATCH_SIZE = +> 32`) forced all tasks onto a single worker thread when the total number of +> projects in the manifest was fewer than 32, completely negating the benefit of +> parallelism for smaller workloads. Failing to adhere to this typically results +> in **Thread Serialization / Under-utilization**. + +**Trap 1: Passing a hardcoded batch integer to the `chunksize` parameter.** + +**Don't:** + +```python +# BAD: Hardcoded chunksize leads to serialization for small input lists +submit(func, inputs, chunksize=WORKER_BATCH_SIZE) +``` + +**Do:** + +```python +# GOOD: Dynamic chunking balances load across workers +calc_chunk = min(max(1, len(inputs) // jobs), WORKER_BATCH_SIZE) +submit(func, inputs, chunksize=calc_chunk) +``` + +**Exceptions:** When evaluating specifically within tests that forcefully +isolate to a single job execution (`jobs=1`). + +-------------------------------------------------------------------------------- + +#### T1-14: Deferred Worktree Operations in Sync Local Half + +> **Rule:** Must enqueue manifest copy and link actions into the synchronization +> buffer explicitly prior to issuing early return signals for up-to-date +> repositories. +> +> **What:** File link (`linkfile`) and copy (`copyfile`) directives must be +> scheduled using the synchronization buffer (`syncbuf.later1`) within the +> `Sync_LocalHalf` execution phase. They must not be bypassed if the repository +> exits its sync early due to being up-to-date. +> +> **Applies To:** Local synchronization logic of projects (`project.py` -> +> `Sync_LocalHalf`). +> +> **Why:** A bug existed where if a project had published commits in Gerrit +> (meaning its local state was merged), the sync process would return early and +> entirely skip applying the manifest's `linkfile` and `copyfile` operations, +> leading to missing or stale files in the developer's worktree. Failing to +> adhere to this typically results in **Worktree Desync / Missing Files**. + +**Trap 1: Returning early from the sync phase without applying required file +operations.** + +**Don't:** + +```python +# BAD: Skipped file modifications when commits are published +if pub == head: + return +``` + +**Do:** + +```python +# GOOD: Schedule file operations via syncbuf before returning +if pub == head: + syncbuf.later1(self, _doff, not verbose) + return +``` + +-------------------------------------------------------------------------------- + +### Cross-Domain Dependencies + +* **Upstream:** T3 | Subprocess Git Integration & Error Translation - + *Standardizes Git command stream capturing and exception typing, enabling + reliable lock contention detection.* +* **Downstream:** T2 | Filesystem Atomicity & Worktree Layout - *Relies on + parallel execution guard clauses to ensure worktrees exist before checkout + operations are dispatched.* +* **Downstream:** T6 | CLI Argument Parsing & UX Consistency - *Requires + buffered standard outputs and structured logger directives from parallel + tasks to maintain coherent terminal formatting.* + +## Chapter: Filesystem Atomicity & Worktree Layout + +**Context:** This domain governs the deterministic creation, migration, and +cleanup of internal repository structures and Git worktrees. It relies on +ephemeral temporary directories, atomic rename operations, and robust error +recovery to prevent corrupted states during unexpected interruptions. + +### Summary + +| Rule ID | Principle / Constraint | Priority | Primary Symptom / | +: : : : Trap : +| :-------- | :---------------------------- | :------- | :-------------------- | +| **T2-01** | Unique Subproject Keys in | High | Using the git | +: : Worktree Environments : : directory or : +: : : : repository name as a : +: : : : unique hash key for : +: : : : deduplicating project : +: : : : objects. : +| **T2-02** | Non-Destructive Bottom-Up | High | Using aggressive | +: : Directory Pruning : : recursive delete : +: : : : calls to wipe an old : +: : : : link destination : +: : : : without verifying its : +: : : : internal contents. : +| **T2-03** | Guaranteed Cleanup of | Critical | Running destructive | +: : Temporary Git Artifacts : : or state-modifying : +: : : : operations without : +: : : : wrapping the cleanup : +: : : : routine in a : +: : : : `finally` block. : +| **T2-04** | Atomic Directory | High | Initializing a | +: : Initialization via Temporary : : complex directory : +: : Worktrees : : structure directly at : +: : : : its final target path : +: : : : without atomicity : +: : : : guarantees. : +| **T2-05** | Pathlib Adoption over os.path | Medium | Relying on heavily | +: : : : nested string-based : +: : : : os.path : +: : : : constructions. : +| **T2-06** | Atomic Directory | Critical | Creating and mutating | +: : Initialization via Temporary : : a system directory at : +: : Renames : : its final, visible : +: : : : destination. : +| **T2-07** | Encapsulation of Transient | High | Swapping out instance | +: : Configuration State : : configuration : +: : : : variables back and : +: : : : forth during setup. : +| **T2-08** | Explicit Temporary Resource | Critical | Keeping a stale | +: : Ownership and Release : : variable reference to : +: : : : a temporary directory : +: : : : after it has been : +: : : : renamed, allowing a : +: : : : deferred cleanup : +: : : : routine to operate on : +: : : : an unowned path. : +| **T2-09** | Configuration Object | High | Using a temporary | +: : Invalidation Post-Rename : : config object to set : +: : : : flags after the : +: : : : underlying directory : +: : : : has been renamed. : +| **T2-10** | Granular Safety Checks for | Critical | Deleting a shared | +: : Destructive Worktree : : object directory : +: : Operations : : immediately upon : +: : : : deleting a single : +: : : : project that uses it. : +| **T2-11** | Explicit Warnings for Legacy | Critical | Silently executing a | +: : Garbage Collection Fallbacks : : legacy fallback for : +: : : : repository safety : +: : : : configurations : +: : : : without informing the : +: : : : user of the potential : +: : : : unreliability. : +| **T2-12** | Atomic Commits for Filesystem | High | Bundling filesystem | +: : Restructuring : : path migrations, : +: : : : structural : +: : : : re-designs, and : +: : : : initialization logic : +: : : : changes into a single : +: : : : Pull : +: : : : Request/Patchset. : +| **T2-13** | Precise Symlink Target | High | Using a generic | +: : Replacement During Submodule : : string `.replace()` : +: : Migration : : to update a portion : +: : : : of a file path. : +| **T2-14** | Isolating Filesystem | High | Executing internal | +: : Migrations from Network-Only : : directory migrations : +: : Operations : : globally prior to : +: : : : downloading network : +: : : : changes. : +| **T2-15** | Idempotent Cross-Platform | High | Relying on standard | +: : File Cleanup : : library file removal : +: : : : and failing to handle : +: : : : the scenario where : +: : : : the file no longer : +: : : : exists. : +| **T2-16** | Self-Healing Binary JSON | High | Opening JSON state | +: : Deserialization : : files in text mode : +: : : : without validating : +: : : : structure, allowing : +: : : : exceptions to bubble : +: : : : up and break the CLI. : +| **T2-17** | Atomic File Writes for | Critical | Using the standard | +: : Repository Configurations : : python `open()` : +: : : : context manager and : +: : : : `print()` to mutate : +: : : : core git directories. : +| **T2-18** | File Descriptor Lock Scope | Medium | Processing strings | +: : Minimization : : and building : +: : : : directory paths : +: : : : inside the file : +: : : : reading scope. : +| **T2-19** | Two-Phase Atomic Deletion for | High | Directly triggering | +: : Worktrees : : `rmtree` on a live : +: : : : repository path. : +| **T2-20** | Conditional Absolute Path | High | Blindly deleting and | +: : Resolution for Git Worktrees : : rewriting the : +: : : : `gitdir` file using : +: : : : `os.path.relpath` : +: : : : without checking if : +: : : : the source path is : +: : : : absolute first. : +| **T2-21** | Independent Manifest | High | Defaulting to a | +: : Repository Shallow Cloning : : shallow clone for : +: : : : configuration : +: : : : repositories, causing : +: : : : subsequent : +: : : : initialization/branch : +: : : : switching commands to : +: : : : fail due to missing : +: : : : objects. : + +-------------------------------------------------------------------------------- + +### Rules + +#### T2-01: Unique Subproject Keys in Worktree Environments + +> **Rule:** Always use repository-relative paths instead of git directory names +> as unique deduplication keys to prevent collisions in worktree environments. +> +> **What:** Repository-relative paths must be used instead of git directory +> names as unique deduplication keys to prevent collisions in environments +> utilizing git worktrees. +> +> **Applies To:** Subproject iteration and deduplication logic, primarily within +> `command.py` or project discovery utilities. +> +> **Why:** When tracking derived subprojects, the system originally used +> `gitdir` as a unique identifier. In git worktree setups, multiple distinct +> project instances can share the same underlying `gitdir`, leading to silent +> data collisions and missed projects during sync operations. Failing to adhere +> to this typically results in **Data Collision / Omission**. + +**Trap 1: Using the git directory or repository name as a unique hash key for +deduplicating project objects.** + +**Don't:** + +```python +derived_projects.update( + (p.gitdir, p) for p in project.GetDerivedSubprojects() +) +``` + +**Do:** + +```python +derived_projects.update( + (p.RelPath(local=False), p) for p in project.GetDerivedSubprojects() +) +``` + +#### T2-02: Non-Destructive Bottom-Up Directory Pruning + +> **Rule:** Always utilize non-destructive, bottom-up directory iteration that +> exclusively targets empty directories and symlinks during legacy path cleanup. +> +> **What:** Legacy path cleanup must utilize non-destructive, bottom-up +> directory iteration that specifically targets empty directories and symlinks, +> avoiding recursive tree destruction. +> +> **Applies To:** Workspace migration, obsolete symlink cleanup, and +> `platform_utils` file deletion wrappers. +> +> **Why:** When a subproject's destination directory changed, the +> synchronization tool utilized aggressive recursive deletion (`rmtree`) to +> remove the previous location. This routinely endangered untracked developer +> code or extraneous files housed inside the obsolete tree. Failing to adhere to +> this typically results in **Accidental Data Deletion**. + +**Trap 1: Using aggressive recursive delete calls to wipe an old link +destination without verifying its internal contents.** + +**Don't:** + +```python +if platform_utils.isdir(absDest): + platform_utils.rmtree(absDest) +``` + +**Do:** + +```python +# Use a safe helper that only removes empty dirs/symlinks bottom-up +platform_utils.removedirs(absDest) +``` + +**Trap 2: Attempting directory deletion without explicitly pruning stale +manifest files first.** + +**Don't:** + +```python +platform_utils.removedirs(need_remove_path) +# Misses files entirely as removedirs stops at regular files +``` + +**Do:** + +```python +if os.path.isfile(need_remove_path) or os.path.islink(need_remove_path): + platform_utils.remove(need_remove_path) +platform_utils.removedirs(os.path.dirname(need_remove_path)) +``` + +**Exceptions:** Explicitly defined cases where a directory is formally tracked +and deleted from the manifest metadata, explicitly commanding removal. + +#### T2-03: Guaranteed Cleanup of Temporary Git Artifacts + +> **Rule:** Must wrap any operation mutating the local `.git` repository with +> temporary states inside a `try/finally` block to guarantee restoration. +> +> **What:** Operations mutating the local `.git` repository with temporary +> states (e.g., tracking alt refs during fetch) must wrap the operation in a +> `try/finally` block to guarantee state restoration. +> +> **Applies To:** Subprocess network operations and filesystem interactions +> affecting the `.git` database. +> +> **Why:** If a network operation threw an exception (like a `GitAuthError` +> prompting for credentials), the cleanup logic was bypassed. This left orphaned +> references in the object database and corrupted future repository syncs. +> Failing to adhere to this typically results in **Stale Repository State**. + +**Trap 1: Running destructive or state-modifying operations without wrapping the +cleanup routine in a `finally` block.** + +**Don't:** + +```python +setup_temporary_refs() +run_git_fetch() # Might throw GitAuthError +cleanup_temporary_refs() +``` + +**Do:** + +```python +setup_temporary_refs() +try: + run_git_fetch() +finally: + cleanup_temporary_refs() +``` + +#### T2-04: Atomic Directory Initialization via Temporary Worktrees + +> **Rule:** Always perform directory initialization via temporary directories +> and an atomic rename to prevent fragmented repository states. +> +> **What:** Directory initialization processes must use temporary directories +> and atomic renames to prevent partial or corrupted states upon unexpected +> interruption. +> +> **Applies To:** Filesystem operations involving git repository setup, +> specifically superproject initialization and dot-git dir creation. +> +> **Why:** Historically, initializing git directories directly in their final +> path left behind broken, partially-initialized structures if the user or +> system interrupted the process. This caused subsequent workflow executions to +> fail permanently. Failing to adhere to this typically results in **Corrupt +> State / Initialization Failure**. + +**Trap 1: Initializing a complex directory structure directly at its final +target path without atomicity guarantees.** + +**Don't:** + +```python +os.makedirs(final_git_dir, exist_ok=True) +subprocess.run(['git', 'init', final_git_dir]) +``` + +**Do:** + +```python +temp_dir = tempfile.mkdtemp(dir=target_base) +try: + subprocess.run(['git', 'init', temp_dir]) + platform_utils.rename(temp_dir, final_git_dir) +finally: + platform_utils.rmtree(temp_dir, ignore_errors=True) +``` + +#### T2-05: Pathlib Adoption over os.path + +> **Rule:** Avoid legacy `os.path` APIs; prefer modern `pathlib.Path` structures +> for traversing, joining, and creating files. +> +> **What:** Use modern Python `pathlib.Path` paradigms for traversing, joining, +> and creating files instead of the legacy `os.path` APIs. +> +> **Applies To:** Filesystem and worktree directory manipulation. +> +> **Why:** Chaining `os.path.join` and calling `os.mkdir` repeatedly bloated the +> codebase, was less readable, and invited platform separator path bugs. Failing +> to adhere to this typically results in **Pathing Errors / High Complexity**. + +**Trap 1: Relying on heavily nested string-based os.path constructions.** + +**Don't:** + +```python +manifest_dir = os.path.join(self.repodir, 'manifests') +os.mkdir(manifest_dir) +with open(os.path.join(manifest_dir, 'config'), 'w') as fp: + fp.write(data) +``` + +**Do:** + +```python +manifest_dir = self.repodir / 'manifests' +manifest_dir.mkdir(parents=True, exist_ok=True) +(manifest_dir / 'config').write_text(data) +``` + +**Exceptions:** Legacy modules pending modernization where intermingling +`os.path` strings and `Path` objects would break typing contracts. + +#### T2-06: Atomic Directory Initialization via Temporary Renames + +> **Rule:** Never construct complex internal `.git` directories in-situ; execute +> initialization inside an ephemeral directory and atomically rename it upon +> completion. +> +> **What:** Complex directory structures (like internal `.git` directories) must +> be constructed inside an ephemeral temporary directory on the same filesystem +> volume, and only moved to their final path via an atomic rename once fully +> validated. +> +> **Applies To:** Filesystem creation (`os.makedirs`) and repository +> initialization (`_InitGitDir`). +> +> **Why:** Interrupted or failed operations left partially initialized `.git` +> directories in-situ. Follow-up operations encountered corrupt directory trees, +> which the legacy recovery logic could not reliably clean up. Failing to adhere +> to this typically results in **Corrupt Repository State**. + +**Trap 1: Creating and mutating a system directory at its final, visible +destination.** + +**Don't:** + +```python +os.makedirs(self.gitdir) +self._ReferenceGitDir(self.objdir, self.gitdir) +``` + +**Do:** + +```python +tmp_gitdir = create_tmp_dir(os.path.dirname(self.gitdir)) +os.makedirs(tmp_gitdir) +self._ReferenceGitDir(self.objdir, tmp_gitdir) +platform_utils.rename(tmp_gitdir, self.gitdir) +``` + +#### T2-07: Encapsulation of Transient Configuration State + +> **Rule:** Never reassign class-level instance attributes to intermediate paths +> during object setup; use local variables for temporary state tracking. +> +> **What:** Never temporarily reassign class-level instance attributes (e.g., +> `self.config`) to point to intermediate or ephemeral paths. Use local +> variables for temporary objects during setup, assigning the final result to +> the class instance only upon success. +> +> **Applies To:** Object initialization workflows within `project.py`. +> +> **Why:** During atomic `.git` directory initialization, `self.config` was +> temporarily overwritten to point to a temporary working directory. If the +> operation crashed, the object was left in an invalid state, pointing to a +> deleted transient path. Failing to adhere to this typically results in +> **Corrupt Instance State**. + +**Trap 1: Swapping out instance configuration variables back and forth during +setup.** + +**Don't:** + +```python +self.config = GitConfig.ForRepository(gitdir=tmp_gitdir) +# initialization steps +platform_utils.rename(tmp_gitdir, self.gitdir) +self.config = GitConfig.ForRepository(gitdir=self.gitdir) +``` + +**Do:** + +```python +tmp_config = GitConfig.ForRepository(gitdir=tmp_gitdir) +# apply setup using tmp_config +platform_utils.rename(tmp_gitdir, self.gitdir) +self.config = GitConfig.ForRepository(gitdir=self.gitdir) +``` + +#### T2-08: Explicit Temporary Resource Ownership and Release + +> **Rule:** Always nullify references to a temporary directory immediately after +> successfully renaming it to its permanent destination. +> +> **What:** When executing atomic directory initializations via temporary paths, +> the reference to the temporary directory must be explicitly nullified +> immediately after successfully renaming it to its final destination. +> +> **Applies To:** Filesystem operations, specifically in `project.py` atomic Git +> directory initializations (`_InitGitDir`). +> +> **Why:** If a temporary directory reference was maintained after being +> atomic-renamed, the subsequent error-handling or `finally` cleanup block could +> mistakenly delete the directory. In parallel environments, another concurrent +> job could validly create a new temporary directory at that exact freed path, +> causing the current process to inadvertently nuke an active resource belonging +> to another job. Failing to adhere to this typically results in **Data Loss / +> Race Condition**. + +**Trap 1: Keeping a stale variable reference to a temporary directory after it +has been renamed, allowing a deferred cleanup routine to operate on an unowned +path.** + +**Don't:** + +```python +try: + tmp_gitdir = tempfile.mkdtemp() + # ... initialize ... + platform_utils.rename(tmp_gitdir, self.gitdir) + # BAD: tmp_gitdir still holds the path string +finally: + if tmp_gitdir and os.path.exists(tmp_gitdir): + platform_utils.rmtree(tmp_gitdir) # Risk of race condition +``` + +**Do:** + +```python +try: + tmp_gitdir = tempfile.mkdtemp() + # ... initialize ... + platform_utils.rename(tmp_gitdir, self.gitdir) + tmp_gitdir = None # GOOD: explicitly release ownership of the path +finally: + if tmp_gitdir and os.path.exists(tmp_gitdir): + platform_utils.rmtree(tmp_gitdir) +``` + +#### T2-09: Configuration Object Invalidation Post-Rename + +> **Rule:** Must discard intermediate configuration instances bound to temporary +> paths once the underlying directory is atomic-renamed. +> +> **What:** Configuration instances (e.g., `GitConfig`) bound to a temporary +> filesystem path must be abandoned after the path is atomic-renamed. A new +> instance tracking the permanent directory path must be used for subsequent +> settings. +> +> **Applies To:** Git config lifecycle management across atomic filesystem +> boundary events. +> +> **Why:** Applying configuration changes like `gc.pruneExpire` using an object +> instantiated against a temporary path resulted in IO errors or lost state if +> the operations occurred after the directory was moved to its final, permanent +> location. Failing to adhere to this typically results in **Lost Configuration +> / IO Error**. + +**Trap 1: Using a temporary config object to set flags after the underlying +directory has been renamed.** + +**Don't:** + +```python +curr_config = GitConfig.ForRepository(gitdir=tmp_gitdir) +platform_utils.rename(tmp_gitdir, self.gitdir) +curr_config.SetString("gc.pruneExpire", "never") # BAD: Path no longer valid +``` + +**Do:** + +```python +curr_config = GitConfig.ForRepository(gitdir=tmp_gitdir) +platform_utils.rename(tmp_gitdir, self.gitdir) +self.config.SetString("gc.pruneExpire", "never") # GOOD: Using permanent config tracking self.gitdir +``` + +#### T2-10: Granular Safety Checks for Destructive Worktree Operations + +> **Rule:** Must evaluate explicit dependency subsets and prompt overrides +> before wiping shared object directories. +> +> **What:** Destructive operations (like wiping directories) must separately +> evaluate and prompt overrides for distinct risk categories (uncommitted local +> changes vs. shared object directories), and only delete a shared object +> directory if all dependent projects are successfully wiped. +> +> **Applies To:** Commands modifying or deleting project directories and +> `.repo/project-objects`. +> +> **Why:** Deleting shared `.repo` state indiscriminately could leave other +> referencing projects broken if they relied on the same object directory, +> requiring strict subset checks before `rmtree` execution. Failing to adhere to +> this typically results in **Repository Corruption / Data Loss**. + +**Trap 1: Deleting a shared object directory immediately upon deleting a single +project that uses it.** + +**Don't:** + +```python +# BAD: Deletes shared objdir, breaking other projects +project.DeleteWorktree(force=True) +if os.path.exists(project.objdir): + platform_utils.rmtree(project.objdir) +``` + +**Do:** + +```python +# GOOD: Verify all users of the objdir are deleted first +project.DeleteWorktree(force=True) +successful_wipes.add(project.relpath) + +# Later... +if users.issubset(successful_wipes): + platform_utils.rmtree(objdir) +``` + +#### T2-11: Explicit Warnings for Legacy Garbage Collection Fallbacks + +> **Rule:** Must emit a visible warning to `sys.stderr` when disabling Git +> garbage collection via legacy fallback configurations. +> +> **What:** When disabling Git garbage collection for shared network +> repositories using legacy mechanisms (`gc.pruneExpire=never`), the tooling +> must emit a visible `sys.stderr` warning due to the unreliability of older Git +> clients. +> +> **Applies To:** Git configuration manipulation and local filesystem management +> (`_GCProjects`). +> +> **Why:** Older Git clients (pre 2.7.0) do not natively support the +> `extensions.preciousObjects` flag. To prevent garbage collection from +> corrupting shared objects, `repo` forces `gc.pruneExpire` to `never`. Because +> this is imperfect, it can lead to unreliable repository states, requiring +> explicit user notification. Failing to adhere to this typically results in +> **Data Loss / Corruption**. + +**Trap 1: Silently executing a legacy fallback for repository safety +configurations without informing the user of the potential unreliability.** + +**Don't:** + +```python +# BAD: Silent fallback to legacy GC directive +if git_require((2, 7, 0)): + project.config.SetString('extensions.preciousObjects', 'true') +else: + project.config.SetString('gc.pruneExpire', 'never') +``` + +**Do:** + +```python +# GOOD: Alerting the user to the legacy system risk +if git_require((2, 7, 0)): + project.config.SetString('extensions.preciousObjects', 'true') +else: + print('WARNING: shared projects are unreliable when using old versions of git...', file=sys.stderr) + project.config.SetString('gc.pruneExpire', 'never') +``` + +#### T2-12: Atomic Commits for Filesystem Restructuring + +> **Rule:** Always strictly isolate structural filesystem modifications and +> migrations into single atomic commits to maintain bisectability. +> +> **What:** Refactoring critical filesystem paths (like migrating internal +> `.git` submodule structures) must be strictly isolated into atomic commits to +> preserve bisectability. +> +> **Applies To:** Worktree layout updates, `.git` metadata migrations, and +> submodule structure changes. +> +> **Why:** A monolithic commit attempted to restructure how submodules were +> stored (from `subprojects/` to `modules/`), update initialization logic, and +> alter fetching. This density made code review difficult and created a high +> risk of regressions that would be impossible to bisect cleanly. Failing to +> adhere to this typically results in **Unbisectable Regressions**. + +**Trap 1: Bundling filesystem path migrations, structural re-designs, and +initialization logic changes into a single Pull Request/Patchset.** + +**Don't:** + +* Submit 1 patchset: [Refactor] Move submodules to modules/, update git config + flags, and modify sync --init behaviors. + +**Do:** + +* Submit 3 sequential patchsets: 1) Isolate init flag changes. 2) Migrate + filesystem directories from `subprojects` to `modules`. 3) Enable Git + recurse submodules integration. + +#### T2-13: Precise Symlink Target Replacement During Submodule Migration + +> **Rule:** Always use explicit path prefixes instead of generic substring +> replacements to update symlink targets during legacy layout migrations. +> +> **What:** Symlink updates during filesystem layout migrations must use exact +> path prefixes rather than global substring replacements to update relative +> targets safely. +> +> **Applies To:** Repository migration logic (`project.py`); specifically +> `_MigrateSubprojectLinks` or similar symlink target modifications. +> +> **Why:** During the migration of legacy submodule directories, using generic +> string replacement (`target.replace`) on symlink paths unintentionally +> corrupted paths if a user's directory structure legitimately contained the +> target string (e.g., `subproject-objects`) in a higher-level parent directory +> name. Failing to adhere to this typically results in **Broken Symlinks / +> Corruption**. + +**Trap 1: Using a generic string `.replace()` to update a portion of a file +path.** + +**Don't:** + +```python +if "subproject-objects" in target: + new_target = target.replace( + "subproject-objects", "module-objects" + ) +``` + +**Do:** + +```python +if target.startswith("../../subproject-objects/"): + new_target = target.replace( + "../../subproject-objects/", "../../module-objects/" + ) +``` + +#### T2-14: Isolating Filesystem Migrations from Network-Only Operations + +> **Rule:** Never execute internal repository layout migrations as part of +> network fetch iterations. +> +> **What:** Internal repository layout migrations must only execute during local +> worktree initialization phases, never during network fetch iterations. +> +> **Applies To:** Repository sync operations (`subcmds/sync.py`) and worktree +> checkout logic (`project.py`). +> +> **Why:** Binding irreversible filesystem layout migrations to the `sync` +> command's network-fetch loop risked partially migrating the repository if the +> process was interrupted, or if a user requested a network-only sync, leaving +> the local workspace out of sync with internal references. Failing to adhere to +> this typically results in **Repository Corruption**. + +**Trap 1: Executing internal directory migrations globally prior to downloading +network changes.** + +**Don't:** + +```python +# In subcmds/sync.py during network fetching +for p in all_projects: + p._MigrateOldSubprojectDirs() + # ... fetch logic +``` + +**Do:** + +```python +# Inside project.py during _InitWorkTree() or checkout +def _InitWorkTree(self): + self._MigrateOldSubmoduleDirs() + # ... symlink creation and checkout logic +``` + +#### T2-15: Idempotent Cross-Platform File Cleanup + +> **Rule:** Must employ `platform_utils.remove()` for file cleanup to safely +> absorb OS access violations and gracefully swallow `ENOENT`. +> +> **What:** File cleanup operations must use the internal +> `platform_utils.remove()` instead of the standard `os.remove()` to handle +> Windows-specific OS limitations. Furthermore, cleanup must be idempotent by +> explicitly catching and swallowing `errno.ENOENT` errors. +> +> **Applies To:** Local worktree operations, internal state file cleanup (e.g., +> deleting obsolete copyfiles or linkfiles). +> +> **Why:** Sync operations would abruptly crash on Windows due to unhandled +> `EACCES` issues on read-only files/symlinks with native `os.remove()`. +> Furthermore, if a tracking file was already deleted manually by the user or a +> prior interrupted run, unhandled `ENOENT` exceptions would needlessly fail the +> operation. Failing to adhere to this typically results in **Crash on +> Missing/Locked File**. + +**Trap 1: Relying on standard library file removal and failing to handle the +scenario where the file no longer exists.** + +**Don't:** + +```python +if path: + try: + os.remove(path) + except OSError as error: + print(f'error: remove {path} failed.') + return 1 +``` + +**Do:** + +```python +try: + platform_utils.remove(path) +except OSError as e: + if e.errno == errno.ENOENT: + # File does not exist, safe to ignore + pass +``` + +#### T2-16: Self-Healing Binary JSON Deserialization + +> **Rule:** Always read internal JSON state files in binary mode (`rb`) and +> implement explicit error catching to seamlessly discard corrupted states. +> +> **What:** Internal JSON tracking files must be read in binary mode (`rb`) to +> allow the JSON parser to detect text encoding safely. Additionally, parsing +> must be wrapped in a `try/except` block to automatically delete the file and +> recover if the JSON is corrupted. +> +> **Applies To:** Manifest metadata tracking files, internal cache parsing +> (e.g., `copy-link-files.json`). +> +> **Why:** When the operating system's default text encoding did not match the +> file's encoding, a `UnicodeDecodeError` could occur. Additionally, if the JSON +> state file was corrupted (e.g., due to an abrupt power loss or killed +> process), `repo sync` would permanently fail until the user manually +> discovered and deleted the corrupted state file. Failing to adhere to this +> typically results in **Corrupt State Deadlock**. + +**Trap 1: Opening JSON state files in text mode without validating structure, +allowing exceptions to bubble up and break the CLI.** + +**Don't:** + +```python +with open(copylinkfiles_path, 'r') as fd: + old_copylinkfiles_path = json.load(fd) +``` + +**Do:** + +```python +with open(copylinkfiles_path, 'rb') as fp: + try: + old_copylinkfiles_path = json.load(fp) + except: + print('error: %s is not a json formatted file.' % copylinkfiles_path, file=sys.stderr) + platform_utils.remove(copylinkfiles_path) + return False +``` + +#### T2-17: Atomic File Writes for Repository Configurations + +> **Rule:** Must utilize atomic file writes rather than standard contexts when +> generating core `.git` configuration files. +> +> **What:** System state modifications, specifically writing the `.git` pointer +> files for submodules, must be performed using an atomic write pattern (e.g., +> writing to a lockfile and renaming) rather than standard file descriptors. +> +> **Applies To:** All file I/O operations directly modifying `.git` contents, +> worktrees, or manifest definitions. +> +> **Why:** Using standard `open()` and writing to `.git` files directly leaves +> the repository vulnerable to corruption if the process is interrupted (e.g., +> via Ctrl+C or SIGTERM) during the write loop, resulting in a partially written +> pointer. Failing to adhere to this typically results in **Corrupted Git +> Worktree**. + +**Trap 1: Using the standard python `open()` context manager and `print()` to +mutate core git directories.** + +**Don't:** + +```python +with open(dotgit, "w") as fp: + print(f"gitdir: {rel_path}", file=fp) +``` + +**Do:** + +```python +_lwrite(dotgit, f"gitdir: {rel_path}\n") +``` + +#### T2-18: File Descriptor Lock Scope Minimization + +> **Rule:** Minimize file descriptor lock retention by executing string and +> state logic outside of standard reading contexts. +> +> **What:** Data processing and variable manipulation must be extracted out of +> file descriptor lock contexts (`with open(...)`) to minimize resource +> retention. +> +> **Applies To:** Any block parsing local text or configuration files. +> +> **Why:** Holding file descriptors open while executing CPU-bound string +> manipulations increases lock contention probability and expands the attack +> surface for race conditions when multiple concurrent processes read/write +> repository states. Failing to adhere to this typically results in **Resource +> Lock Contention**. + +**Trap 1: Processing strings and building directory paths inside the file +reading scope.** + +**Don't:** + +```python +with open(dotgit) as fp: + setting = fp.read() + gitdir = setting.split(":")[1].strip() + dotgit_path = os.path.normpath(os.path.join(self.worktree, gitdir)) +``` + +**Do:** + +```python +with open(dotgit) as fp: + setting = fp.read() + +# Lock released, safe to process +gitdir = setting.split(":")[1].strip() +dotgit_path = os.path.normpath(os.path.join(self.worktree, gitdir)) +``` + +#### T2-19: Two-Phase Atomic Deletion for Worktrees + +> **Rule:** Must enact permanent `.git` and worktree removal via a two-phase +> destruction pattern utilizing a temporary trash path. +> +> **What:** When permanently removing a `.git` directory or worktree, the system +> must first perform an atomic rename to a temporary trash path before +> initiating the recursive deletion. +> +> **Applies To:** Garbage collection (`gc.py`), worktree cleanup, and submodule +> removal logic. +> +> **Why:** Recursive deletion is not instantaneous. If a user forcefully +> interrupted (Ctrl+C) the `rmtree` operation, the directory was left in a +> 'wedged' (partially deleted but still registered) state, permanently breaking +> future syncs for that repository. Failing to adhere to this typically results +> in **Wedged / Orphaned Worktree**. + +**Trap 1: Directly triggering `rmtree` on a live repository path.** + +**Don't:** + +```python +for path in to_delete: + platform_utils.rmtree(path) +``` + +**Do:** + +```python +for path in to_delete: + temp_path = rename_to_trash(path) + platform_utils.rmtree(temp_path) +``` + +#### T2-20: Conditional Absolute Path Resolution for Git Worktrees + +> **Rule:** Always independently verify if a tracked worktree path is absolute +> before initiating internal path recreation procedures. +> +> **What:** Updates to `.git` directory reference files (e.g., `gitdir:`) must +> verify whether the stored worktree path is absolute before attempting to +> recreate it as a relative path to avoid path corruption and permissions +> errors. +> +> **Applies To:** Internal Git directory layout management, specifically +> cross-platform Git worktree initialization (`project.py`). +> +> **Why:** On certain platforms like Windows, modifying the internal `dotgit` +> file in situ fails due to file permissions. Code was added to delete and +> recreate the file using relative paths. However, depending on the Git version, +> the initial path could already be relative, making conversion unsafe without +> an `isabs()` check. Failing to adhere to this typically results in +> **Permission Denied / Path Corruption**. + +**Trap 1: Blindly deleting and rewriting the `gitdir` file using +`os.path.relpath` without checking if the source path is absolute first.** + +**Don't:** + +```python +platform_utils.remove(dotgit) +with open(dotgit, "w", newline="\n") as fp: + print("gitdir:", os.path.relpath(git_worktree_path, self.worktree), file=fp) +``` + +**Do:** + +```python +if os.path.isabs(git_worktree_path): + platform_utils.remove(dotgit) + with open(dotgit, "w", newline="\n") as fp: + print("gitdir:", os.path.relpath(git_worktree_path, self.worktree), file=fp) +``` + +#### T2-21: Independent Manifest Repository Shallow Cloning + +> **Rule:** Never propagate the primary manifest `--depth` flag to govern the +> core manifest repository's clone boundaries. +> +> **What:** The depth configuration for cloning the central manifest repository +> must be explicitly decoupled from the depth configuration applied to the child +> projects it governs, and its default must not regress existing multi-stage +> init workflows. +> +> **Applies To:** Repo initialization commands (`repo init`, `subcmds/init.py`). +> +> **Why:** Using the global `--depth` flag inadvertently forced shallow clones +> on all child projects. Introducing a dedicated `--manifest-depth` option fixed +> this, but defaulting it to 1 caused a regression during 'double repo init' +> workflows (where subsequent checkouts failed against truncated manifest +> histories). Failing to adhere to this typically results in **Workflow +> Regression / History Truncation**. + +**Trap 1: Defaulting to a shallow clone for configuration repositories, causing +subsequent initialization/branch switching commands to fail due to missing +objects.** + +**Don't:** + +```python +group.add_option('--manifest-depth', type='int', default=1, metavar='DEPTH', + help='create a shallow clone of the manifest repo') +``` + +**Do:** + +```python +group.add_option('--manifest-depth', type='int', default=0, metavar='DEPTH', + help='create a shallow clone of the manifest repo') +``` + +-------------------------------------------------------------------------------- + +### Cross-Domain Dependencies + +* **Upstream:** T3 | Subprocess Git Integration & Error Translation - *Network + operations and git process invocations require safe staging paths and + catchable exceptions to trigger filesystem cleanups.* +* **Downstream:** T1 | Concurrent Synchronization & IPC - *Parallel checkout + operations depend heavily on atomic path modifications and lock-free + directory states to prevent race conditions.* +* **Downstream:** T4 | Manifest Object Model & Deduplication - *Filesystem + migrations and subproject tree building directly parse and reflect + canonicalized XML manifest declarations.* + +## Chapter: Subprocess Git Integration & Error Translation + +**Context:** This chapter defines the constraints for wrapping, executing, and +translating standard Git subprocesses within the Repo tooling ecosystem. It +mandates the use of centralized command abstractions, strict version gating, and +deterministic stream handling to guarantee reliable repository state management +and actionable error reporting. + +### Summary + +| Rule ID | Principle / Constraint | Priority | Primary Symptom / Trap | +| :-------- | :------------------------ | :------- | :------------------------ | +| **T3-01** | Unborn Branch Resolution | High | Catching a `rev-parse` | +: : via Symbolic-Ref : : GitError and immediately : +: : : : falling back to manual : +: : : : file system reads or : +: : : : emitting warnings. : +| **T3-02** | Centralized GitCommand | Medium | Wrapping `subprocess.run` | +: : Abstraction Usage : : manually inside a class : +: : : : to invoke Git. : +| **T3-03** | NUL-Byte Delimiters for | High | Formatting git fields | +: : Git Output Parsing : : with tabs and defending : +: : : : against length : +: : : : variations. : +| **T3-04** | Subprocess Execution | Medium | Stringifying Path | +: : Context and Native Path : : objects, using : +: : Passing : : Git-specific directory : +: : : : shifts, and suppressing : +: : : : piped output. : +| **T3-05** | Runtime Git Version | High | Invoking a modern git CLI | +: : Constraints for Promisor : : flag without ensuring : +: : Packs : : compatibility first. : +| **T3-06** | Centralized Dependency | Medium | Catching missing binary | +: : Availability Caching : : exceptions to terminate : +: : : : the process instead of : +: : : : returning control to the : +: : : : caller. : +| **T3-07** | Subprocess Return Code | High | Calling a non-existent | +: : Validation : : subprocess execution : +: : : : method on an already : +: : : : completed process object. : +| **T3-08** | Strict Git Version | High | Injecting convenient, | +: : Compatibility : : modern CLI flags into : +: : : : subprocess wrappers. : +| **T3-09** | Rebase Override for | High | Throwing a | +: : Published Local Branches : : `LocalSyncFail` when an : +: : : : upstream gain exists, : +: : : : completely ignoring the : +: : : : `force_rebase` argument. : +| **T3-10** | SSO Authentication | High | Treating all exit code | +: : Failure Translation : : 128 errors equally : +: : : : without sniffing stdout : +: : : : for authentication prompt : +: : : : aborts. : +| **T3-11** | Direct Read Fallback for | High | Raising a fatal internal | +: : Unborn Git HEAD : : exception immediately : +: : : : upon a `GitError` when : +: : : : checking for the HEAD : +: : : : reference. : +| **T3-12** | Safe Git Subprocess | Critical | Assuming subprocess | +: : Output Handling : : success and attempting to : +: : : : return an output variable : +: : : : that hasn't been : +: : : : initialized. : +| **T3-13** | Shallow Clone SHA-1 Fetch | Critical | Assuming a fetch of an | +: : Fallback : : arbitrary SHA-1 will : +: : : : unconditionally succeed : +: : : : if `--depth` is used. : +| **T3-14** | Validating Gerrit Push | High | Constructing push options | +: : Option Syntax : : with incorrect, : +: : : : unverified key strings. : +| **T3-15** | Submodule Initialization | Critical | Blindly initializing | +: : Guard Under Git Worktrees : : submodules without : +: : : : verifying if the : +: : : : underlying Git worktree : +: : : : feature is active. : +| **T3-16** | XDG Compliant Git | High | Hardcoding the user's | +: : Configuration Resolution : : home directory as the : +: : : : sole location for : +: : : : `.gitconfig`. : +| **T3-17** | Strongly-Typed Subprocess | High | Catching a subprocess | +: : Error Contexting : : failure and re-raising it : +: : : : as a generic git error : +: : : : with a concatenated : +: : : : string message. : +| **T3-18** | Line-Based Log Extraction | Medium | Truncating stdout to a | +: : for Subprocess : : fixed number of : +: : Diagnostics : : characters and ignoring : +: : : : stderr entirely. : + +-------------------------------------------------------------------------------- + +### Rules + +#### T3-01: Unborn Branch Resolution via Symbolic-Ref + +> **Rule:** Always attempt `git symbolic-ref` before falling back to manual file +> parsing to correctly resolve unborn branches. +> +> **What:** When querying local Git branch structures, `git symbolic-ref` must +> be attempted as a fallback before file parsing to correctly identify unborn +> branches where `rev-parse` fails. +> +> **Applies To:** Subprocess Git integrations resolving `HEAD` or local refs. +> +> **Why:** Freshly initialized repositories and orphan branches present 'unborn' +> states where standard `rev-parse` commands returned errors, triggering +> false-positive warnings or crashing the client. Failing to adhere to this +> typically results in **Incorrect Branch Detection**. + +**Trap 1: Catching a `rev-parse` GitError and immediately falling back to manual +file system reads or emitting warnings.** + +**Don't:** + +```python +try: + return rev_parse(HEAD) +except GitError: + logger.warning("Unparseable HEAD") + return parse_file(HEAD) +``` + +**Do:** + +```python +try: + return rev_parse(HEAD) +except GitError: + try: + return run_git_command("symbolic-ref", "-q", HEAD) + except GitError: + pass + return parse_file(HEAD) +``` + +-------------------------------------------------------------------------------- + +#### T3-02: Centralized GitCommand Abstraction Usage + +> **Rule:** Strictly use the internal `GitCommand` abstraction for local Git +> operations instead of invoking raw `subprocess.run`. +> +> **What:** Use the internal `GitCommand` abstraction for executing Git +> operations rather than raw `subprocess.run` to guarantee consistent tracing, +> environment handling, and error translations. +> +> **Applies To:** Any module interacting with local git binaries, such as +> `git_refs.py`. +> +> **Why:** Developers bypassing the centralized wrapper led to duplicated +> subprocess execution logic, loss of global tracing capabilities (e.g., +> `REPO_TRACE`), and fragmented error handling across the codebase. Failing to +> adhere to this typically results in **Missing Tracing / Environment Bugs**. + +**Trap 1: Wrapping `subprocess.run` manually inside a class to invoke Git.** + +**Don't:** + +```python +def _Run(self, *cmd): + return subprocess.run( + ['git', f'--git-dir={self._gitdir}', *cmd], + stdout=subprocess.PIPE, + stderr=subprocess.DEVNULL + ) +``` + +**Do:** + +```python +from git_command import GitCommand + +def _Run(self, cmd): + p = GitCommand( + None, + cmd, + capture_stdout=True, + capture_stderr=True, + bare=True, + gitdir=self._gitdir, + ) + p.Wait() + return p +``` + +**Exceptions:** Low-level testing utilities isolated from the core application +state, where bypassing `GitCommand` is necessary to bootstrap a test repository. + +-------------------------------------------------------------------------------- + +#### T3-03: NUL-Byte Delimiters for Git Output Parsing + +> **Rule:** Always specify NUL-byte delimiters (`%00`) when parsing Git command +> output to prevent splitting errors. +> +> **What:** When parsing Git formatted output, utilize the NUL byte (`%00`) +> instead of tabs or spaces, and leverage precise tuple unpacking without length +> validation. +> +> **Applies To:** Parsing output from Git commands like `git for-each-ref` or +> `git status`. +> +> **Why:** Using whitespace or tabs to split field data exposed the parser to +> failures if reference names or other entities inherently included those +> characters. NUL delimiters guarantee absolute safety and allow removal of +> defensive parsing loops. Failing to adhere to this typically results in **Data +> Parsing Failure**. + +**Trap 1: Formatting git fields with tabs and defending against length +variations.** + +**Don't:** + +```python +# Using tab delimiter and checking len +subprocess.run(['git', 'for-each-ref', '--format=%(objectname)\t%(refname)']) +for line in output.splitlines(): + fields = line.split('\t') + if len(fields) < 2: + continue + ref_id, name = fields[:2] +``` + +**Do:** + +```python +# Using NUL delimiter and direct unpacking +GitCommand(..., ['for-each-ref', '--format=%(objectname)%00%(refname)%00%(symref)']) +for line in output.splitlines(): + ref_id, name, symref = line.split('\0') +``` + +-------------------------------------------------------------------------------- + +#### T3-04: Subprocess Execution Context and Native Path Passing + +> **Rule:** Pass native `pathlib.Path` objects via the `cwd` argument rather +> than stringifying them or utilizing Git's `-C` flag. +> +> **What:** Execute subprocesses with correct internal API paradigms: omit Git's +> `-C` in favor of the `cwd=` parameter, pass `pathlib.Path` objects inherently, +> and drop `-q` when actively capturing output. +> +> **Applies To:** Subprocess command building and execution in test scaffolding +> and Git interactions. +> +> **Why:** Passing `-C` combined with forced string casting of paths created +> redundant logic. Additionally, suppressing underlying git output via `-q` +> completely masked the root cause of errors captured within the internal stream +> buffers. Failing to adhere to this typically results in **Obscured +> Diagnostics**. + +**Trap 1: Stringifying Path objects, using Git-specific directory shifts, and +suppressing piped output.** + +**Don't:** + +```python +subprocess.check_call( + ['git', '-C', str(git_dir), 'init', '-q'], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE +) +``` + +**Do:** + +```python +subprocess.check_call( + ['git', 'init'], + cwd=git_dir, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE +) +``` + +-------------------------------------------------------------------------------- + +#### T3-05: Runtime Git Version Constraints for Promisor Packs + +> **Rule:** Explicitly verify local Git version compatibility before injecting +> modern CLI flags into subprocess calls. +> +> **What:** Operations utilizing modern Git arguments like +> `--missing=allow-promisor` must be explicitly gated by a minimal supported +> version check against the local executable. +> +> **Applies To:** Subprocess handlers for `git rev-list` and repacking +> mechanisms on partial clones. +> +> **Why:** Git 2.17.0 introduced advanced promisor arguments. Hardcoding these +> arguments in the execution engine crashed older repository clients on machines +> where Git had been downgraded or not updated. Failing to adhere to this +> typically results in **Subprocess Crash**. + +**Trap 1: Invoking a modern git CLI flag without ensuring compatibility first.** + +**Don't:** + +```python +if opt.repack: + return self.repack_projects(projects, opt) +``` + +**Do:** + +```python +if opt.repack: + git_require((2, 17, 0), fail=True, msg="--repack") + return self.repack_projects(projects, opt) +``` + +-------------------------------------------------------------------------------- + +#### T3-06: Centralized Dependency Availability Caching + +> **Rule:** Centralize and cache external dependency checks to fail gracefully +> without abruptly terminating the calling process. +> +> **What:** External dependency checks (e.g., verifying SSH installation) must +> be centralized, cached internally, and fail gracefully without forcibly +> exiting the calling process. +> +> **Applies To:** System API wrappers and subprocess managers (e.g., +> `ProxyManager`, `ssh.py`). +> +> **Why:** Deep utility functions checking for external binaries previously +> called `sys.exit(1)` upon `FileNotFoundError`. This abruptly terminated the +> execution for users in environments missing the dependency, even if their +> specific workflow did not actually require it. Failing to adhere to this +> typically results in **Unexpected Process Termination**. + +**Trap 1: Catching missing binary exceptions to terminate the process instead of +returning control to the caller.** + +**Don't:** + +```python +def version(): + try: + return _parse_ssh_version() + except FileNotFoundError: + print("fatal: ssh not installed", file=sys.stderr) + sys.exit(1) +``` + +**Do:** + +```python +def version(): + try: + return _parse_ssh_version() + except FileNotFoundError as e: + print("warn: ssh not installed", file=sys.stderr) + raise e +``` + +**Trap 2: Duplicating inline `try-except` blocks for missing binaries across +multiple execution methods rather than centralizing state.** + +**Don't:** + +```python +def run_command1(self): + try: + subprocess.Popen(['ssh']) + except FileNotFoundError: + return False + +def run_command2(self): + try: + subprocess.Popen(['ssh']) + except FileNotFoundError: + return False +``` + +**Do:** + +```python +def __enter__(self): + try: + version() + self._ssh_installed = True + except FileNotFoundError: + self._ssh_installed = False + return self + +def run_command(self): + if not self._ssh_installed: + return False + subprocess.Popen(['ssh']) +``` + +-------------------------------------------------------------------------------- + +#### T3-07: Subprocess Return Code Validation + +> **Rule:** Validate subprocess success strictly using `check_returncode()` on +> the completed process object. +> +> **What:** When utilizing the standard Python `subprocess` library, the correct +> method to validate execution success on a `CompletedProcess` object is +> `check_returncode()`. Do not call `check_call()` on the returned object. +> +> **Applies To:** Any Python code wrapping git commands and handling +> `subprocess.CompletedProcess` results. +> +> **Why:** Legacy code inadvertently called `check_call()` on the result of +> `run_command`, which triggered a runtime `AttributeError` instead of raising +> the expected `subprocess.CalledProcessError` on failure. Failing to adhere to +> this typically results in **AttributeError**. + +**Trap 1: Calling a non-existent subprocess execution method on an already +completed process object.** + +**Don't:** + +```python +ret = subprocess.run(cmd, capture_output=True) +ret.check_call() # Raises AttributeError +``` + +**Do:** + +```python +ret = subprocess.run(cmd, capture_output=True) +# This will raise subprocess.CalledProcessError for us +ret.check_returncode() +``` + +-------------------------------------------------------------------------------- + +#### T3-08: Strict Git Version Compatibility + +> **Rule:** Never pass Git flags that are unsupported by the minimum required +> Git version listed in the project's manifest. +> +> **What:** Do not pass modern CLI flags to subprocess Git operations if those +> flags are not supported by the minimum required Git version listed in the +> project's dependency manifest. +> +> **Applies To:** Git subprocess string assembly in `project.py`. +> +> **Why:** The `--no-auto-gc` flag was appended to clone commands to prevent +> deadlocks, but this flag did not exist in Git 1.7.9, immediately breaking +> checkout operations for environments restricted to the minimum supported +> baseline. Failing to adhere to this typically results in **Unknown Option +> Error**. + +**Trap 1: Injecting convenient, modern CLI flags into subprocess wrappers.** + +**Don't:** + +```python +cmd.append("--no-auto-gc") +``` + +**Do:** + +* Disabling auto-gc via configuration setters (`self.config.SetInt("gc.auto", + 0)`) that are safely interpreted across legacy Git versions. + +-------------------------------------------------------------------------------- + +#### T3-09: Rebase Override for Published Local Branches + +> **Rule:** Allow rewriting of published local commits by respecting the +> `force_rebase` argument during sync operations. +> +> **What:** When a local branch is published but not yet merged upstream, `repo +> sync --rebase` must respect the `force_rebase` argument to allow rewriting +> published commits, rather than unconditionally throwing a sync failure. +> +> **Applies To:** `project.py`, specifically `_Rebase` and `_FastForward` path +> execution during sync. +> +> **Why:** The tool previously threw an error instructing the user to run with +> `--rebase`, but didn't actually execute the rebase when the flag was provided, +> forcing developers to manually fix the state. Failing to adhere to this +> typically results in **Broken UX / Unhandled Rebase State**. + +**Trap 1: Throwing a `LocalSyncFail` when an upstream gain exists, completely +ignoring the `force_rebase` argument.** + +**Don't:** + +```python +if upstream_gain and not force_rebase: + # Fails even if force_rebase is TRUE due to logic gap + fail(LocalSyncFail("branch is published... rerun with the --rebase option")) +``` + +**Do:** + +```python +if upstream_gain: + if force_rebase: + syncbuf.later1(self, _dorebase, not verbose) + else: + fail(LocalSyncFail("branch is published... rerun with the --rebase option")) +``` + +-------------------------------------------------------------------------------- + +#### T3-10: SSO Authentication Failure Translation + +> **Rule:** Intercept Git exit code 128 and translate it to a typed +> `GitAuthError` when stdout indicates an SSO session abort. +> +> **What:** Git commands that fail with exit code 128 due to an SSO session +> abort must be explicitly intercepted and translated into a typed +> `GitAuthError`, preventing them from being masked as a missing reference. +> +> **Applies To:** project.py, Git Subprocess Wrappers evaluating `gitcmd.stdout` +> and return codes (specifically fetch/sync operations interacting with SSO +> remote helpers). +> +> **Why:** In non-interactive environments, an expired SSO token caused Git to +> fail with a generic exit code 128. The tool misinterpreted this as a missing +> upstream reference, leading to confusing error logs that obscured the +> underlying authentication requirement. Failing to adhere to this typically +> results in **Ambiguous Auth Failure**. + +**Trap 1: Treating all exit code 128 errors equally without sniffing stdout for +authentication prompt aborts.** + +**Don't:** + +```python +# BAD: Ambiguous handling of 128 +elif current_branch_only and is_sha1 and ret == 128: + # Assuming missing upstream reference + raise GitError("Couldn't find the ref you asked for") +``` + +**Do:** + +```python +# GOOD: Explicit trap for SSO helpers +elif ret == 128 and gitcmd.stdout and "remote helper 'sso' aborted session" in gitcmd.stdout: + raise GitAuthError(gitcmd.stdout) +``` + +**Exceptions:** Repositories containing multiple remotes where an auth failure +on a secondary remote might be non-fatal. (Currently flagged for future review +but translation stands). + +-------------------------------------------------------------------------------- + +#### T3-11: Direct Read Fallback for Unborn Git HEAD + +> **Rule:** Implement a direct file system read of `.git/HEAD` as a fallback +> when standard sub-commands fail to resolve the reference. +> +> **What:** When standard Git commands fail to resolve the current HEAD (e.g., +> throwing a `GitError`), the system must fall back to directly reading the +> internal `.git/HEAD` file from the filesystem before propagating a fatal +> error. +> +> **Applies To:** project.py, `GetHead()` implementations traversing repository +> state. +> +> **Why:** Empty repositories or states featuring unborn branches caused `git +> rev-parse HEAD` to fail outright. Without a fallback, this cascaded into a +> `NoManifestException` and aborted the entire synchronization process. Falling +> back to the raw `.git/HEAD` text bypassed the strict ref checks. Failing to +> adhere to this typically results in **Hard Crash / NoManifestException**. + +**Trap 1: Raising a fatal internal exception immediately upon a `GitError` when +checking for the HEAD reference.** + +**Don't:** + +```python +# BAD: Strict reliance on git sub-commands +try: + return self.rev_parse(HEAD) +except GitError as e: + path = self.GetDotgitPath(subpath=HEAD) + raise NoManifestException(path, str(e)) +``` + +**Do:** + +```python +# GOOD: Fallback text parsing mechanism +try: + return self.rev_parse(HEAD) +except GitError as e: + path = self.GetDotgitPath(subpath=HEAD) + try: + with open(path) as fd: + # Read branch manually + except OSError: + raise NoManifestException(path, str(e)) +``` + +**Exceptions:** If the physical `.git/HEAD` file raises an `OSError` (meaning it +truly does not exist on disk), a `NoManifestException` is necessary and valid. + +-------------------------------------------------------------------------------- + +#### T3-12: Safe Git Subprocess Output Handling + +> **Rule:** Always validate the return code of a Git subprocess before accessing +> its standard output properties. +> +> **What:** Git subprocess output wrappers must explicitly handle non-zero exit +> codes to prevent accessing uninitialized variables, and stream properties must +> be spell-checked against the underlying class definitions. +> +> **Applies To:** Git subprocess wrappers (`git_superproject.py`, `GitCommand`) +> and any execution context capturing stdout/stderr. +> +> **Why:** When a Git command (like `git rev-parse`) failed, the code attempted +> to return a variable (`data`) that was only initialized on success, causing an +> `UnboundLocalError`. Furthermore, error logging attempts crashed due to a typo +> accessing `p.stdwerr` instead of `p.stderr`. Failing to adhere to this +> typically results in **Runtime Crash / UnboundLocalError**. + +**Trap 1: Assuming subprocess success and attempting to return an output +variable that hasn't been initialized.** + +**Don't:** + +```python +# BAD: data is unbound if retval != 0 +retval = p.Wait() +if retval == 0: + data = p.stdout +else: + log_error("Failed") +return data +``` + +**Do:** + +```python +# GOOD: Explicit error handling with early return +retval = p.Wait() +if retval != 0: + log_error(f"Failed: {p.stderr}") + return None +return p.stdout +``` + +**Trap 2: Typo in the attribute name for capturing standard error from the +`GitCommand` class.** + +**Don't:** + +```python +# BAD: Invalid attribute triggers AttributeError +self._LogWarning("Error: {}", p.stdwerr) +``` + +**Do:** + +```python +# GOOD: Correct attribute reference +self._LogWarning("Error: {}", p.stderr) +``` + +-------------------------------------------------------------------------------- + +#### T3-13: Shallow Clone SHA-1 Fetch Fallback + +> **Rule:** Provide a full-sync fallback when shallow fetch requests for +> specific SHA-1s are rejected by server policies. +> +> **What:** Git fetch requests targeting specific SHA-1s with `--depth` enabled +> must handle graceful fallbacks, as server-side policies often restrict +> unadvertised object fetching. +> +> **Applies To:** Git fetch routines in `project.py`, specifically during +> incremental or optimized shallow clones. +> +> **Why:** When optimizing syncs by directly fetching specific SHA-1s with a +> depth limitation, operations failed silently or returned generic errors +> because some Git servers do not advertise all objects, explicitly rejecting +> such requests. Failing to adhere to this typically results in **Sync +> Failure**. + +**Trap 1: Assuming a fetch of an arbitrary SHA-1 will unconditionally succeed if +`--depth` is used.** + +**Don't:** + +```python +# BAD: Missing fallback for unadvertised objects +ret = fetch_sha1(depth=1) +if ret < 0: + break +``` + +**Do:** + +```python +# GOOD: Explicit fallback to a full sync (depth-less) if the specific SHA-1 is rejected +ret = fetch_sha1(depth=1) +if depth and is_sha1 and ret == 1: + # Server may not allow fetching unadvertised refs, fallback to full sync + break +``` + +**Exceptions:** Servers specifically configured to allow +`uploadpack.allowReachableSHA1InWant` or similar parameters, though the client +cannot assume this. + +-------------------------------------------------------------------------------- + +#### T3-14: Validating Gerrit Push Option Syntax + +> **Rule:** Pass custom Gerrit push options strictly using the documented +> `custom-keyed-value` parameter syntax. +> +> **What:** When passing custom metadata to Gerrit via Git push options, +> strictly adhere to the `custom-keyed-value` syntax as defined by the remote +> server documentation. +> +> **Applies To:** `upload.py` operations mapping superproject state to `git push +> -o`. +> +> **Why:** The client attempted to attach superproject metadata to push +> operations using `custom-key-value`, which was ignored by the Gerrit server +> because the documented parameter was `custom-keyed-value`. Failing to adhere +> to this typically results in **Metadata Loss / Server Rejection**. + +**Trap 1: Constructing push options with incorrect, unverified key strings.** + +**Don't:** + +```python +# BAD: Incorrect key format +push_options.append(f"custom-key-value=rootRepo:{host}/{sp_name}") +``` + +**Do:** + +```python +# GOOD: Matches external documentation +push_options.append(f"custom-keyed-value=rootRepo:{host}/{sp_name}") +``` + +-------------------------------------------------------------------------------- + +#### T3-15: Submodule Initialization Guard Under Git Worktrees + +> **Rule:** Skip Git submodule initialization sequences if the repository is +> actively utilizing Git worktrees. +> +> **What:** Submodule initialization operations (`git submodule init`) must be +> explicitly skipped if the repository is configured to use Git worktrees. +> +> **Applies To:** Git command wrappers, worktree initialization sequences, and +> submodule lifecycle operations in `project.py`. +> +> **Why:** Upstream Git has known bugs and incomplete feature support regarding +> the use of submodules within worktrees. Attempting to initialize submodules +> under these conditions leads to undefined behavior and broken repository +> states. Failing to adhere to this typically results in **Upstream Git Bug / +> Inconsistency**. + +**Trap 1: Blindly initializing submodules without verifying if the underlying +Git worktree feature is active.** + +**Don't:** + +```python +self._InitWorkTree(force_sync=force_sync, submodules=submodules) +if self.has_subprojects: + self._InitSubmodules() +``` + +**Do:** + +```python +self._InitWorkTree(force_sync=force_sync, submodules=submodules) +# Avoid doing a submodule init when using worktrees as the support for +# submodules is incomplete. +if self.has_subprojects and not self.use_git_worktrees: + self._InitSubmodules() +``` + +-------------------------------------------------------------------------------- + +#### T3-16: XDG Compliant Git Configuration Resolution + +> **Rule:** Resolve user Git configuration files natively honoring the +> `XDG_CONFIG_HOME` environment variable before defaulting to legacy paths. +> +> **What:** Git configuration lookups must natively respect the +> `XDG_CONFIG_HOME` environment variable, falling back to `~/.config/git/config` +> before finally querying the legacy `~/.gitconfig` location. +> +> **Applies To:** Git configuration parsing, environment setup, and hermetic +> tests. +> +> **Why:** Users who migrated their dotfiles to the XDG Base Directory +> Specification experienced issues because internal tooling rigidly expected +> `~/.gitconfig` to exist, bypassing user intentions and leading to +> unauthenticated or unconfigured repository actions. Failing to adhere to this +> typically results in **Orphaned Configuration State**. + +**Trap 1: Hardcoding the user's home directory as the sole location for +`.gitconfig`.** + +**Don't:** + +```python +def _getUserConfig(): + return os.path.expanduser("~/.gitconfig") +``` + +**Do:** + +```python +def _getUserConfig(): + """This matches git: https://git-scm.com/docs/git-config#FILES""" + xdg_config_home = os.getenv("XDG_CONFIG_HOME", os.path.expanduser("~/.config")) + xdg_config_file = os.path.join(xdg_config_home, "git", "config") + if os.path.exists(xdg_config_file): + return xdg_config_file + return os.path.expanduser("~/.gitconfig") +``` + +-------------------------------------------------------------------------------- + +#### T3-17: Strongly-Typed Subprocess Error Contexting + +> **Rule:** Raise strongly-typed `GitCommandError` exceptions with granular +> state context instead of generic Git errors. +> +> **What:** Git commands that yield non-zero exit codes must raise a specialized +> `GitCommandError` containing granular execution context, rather than a generic +> `GitError`. +> +> **Applies To:** Subprocess execution wrappers within `git_command.py` and +> downstream consumers handling Git process states. +> +> **Why:** When commands failed silently or raised generic errors, downstream +> handlers could not differentiate between configuration errors and actual +> subprocess failures, leading to loss of arguments and return codes. Failing to +> adhere to this typically results in **Lost Diagnostic Context**. + +**Trap 1: Catching a subprocess failure and re-raising it as a generic git error +with a concatenated string message.** + +**Don't:** + +```python +if self.rc != 0: + raise GitError("%s: %s" % (command[1], e)) +``` + +**Do:** + +```python +if self.rc != 0: + raise GitCommandError( + message="git command failure", + project=project.name, + command_args=self.cmdv, + git_rc=self.rc, + git_stdout=stdout, + ) +``` + +-------------------------------------------------------------------------------- + +#### T3-18: Line-Based Log Extraction for Subprocess Diagnostics + +> **Rule:** Extract and log the first meaningful line of both standard output +> and error streams rather than enforcing arbitrary truncation limits. +> +> **What:** When capturing subprocess logs for error reporting, extract the +> first meaningful line of `stdout` and `stderr` instead of relying on arbitrary +> character limits. +> +> **Applies To:** Error truncation logic for Git subprocesses in `Wait()` +> blocks. +> +> **Why:** Truncating output by an arbitrary number of characters (e.g., 80 +> chars) occasionally cut off the actual error string. Furthermore, ignoring +> `stderr` hid the root cause of the subprocess failure. Failing to adhere to +> this typically results in **Obfuscated Error Logs**. + +**Trap 1: Truncating stdout to a fixed number of characters and ignoring stderr +entirely.** + +**Don't:** + +```python +stdout = self.stdout[:80] if self.stdout else None +``` + +**Do:** + +```python +# Prefer extracting the first full line and including stderr +stdout_line = self.stdout.splitlines()[0] if self.stdout else None +stderr_line = self.stderr.splitlines()[0] if self.stderr else None +``` + +-------------------------------------------------------------------------------- + +### Cross-Domain Dependencies + +* **Upstream:** T2 | Filesystem Atomicity & Worktree Layout - *Subprocess + executions must coordinate with filesystem atomicity rules, particularly + regarding submodule paths and worktree usage to prevent upstream Git bugs.* +* **Downstream:** T5 | Hermetic Testing & Test Modernization - *Testing + frameworks depend on hermetic GitCommand wrappers and mocked stream capture + to validate subprocess behaviors safely.* + +## Chapter: Manifest Object Model & Deduplication + +**Context:** This chapter governs the parsing, validation, and canonicalization +of XML manifest components. It strictly enforces semantic immutability via +`NamedTuple` implementations, defensive copying for hierarchical override +scoping, and deterministic JSON-backed file tracking across the subsystem. + +### Summary + +| Rule ID | Principle / Constraint | Priority | Primary Symptom / Trap | +| :-------- | :------------------------ | :------- | :------------------------ | +| **T4-01** | Strict Traversal Prefix | High | Using broad substring | +: : Matching for Relative : : matching to detect : +: : URLs : : relative traversal : +: : : : sequences. : +| **T4-02** | Extend-Project | High | Omitting group | +: : Inheritance Scoping : : inheritance constraints : +: : : : for dynamically : +: : : : overridden node elements. : +| **T4-03** | Tuple Immutability in | Medium | Instantiating transient | +: : Membership Checks : : list objects inside loop : +: : : : conditions. : +| **T4-04** | Defensive Copying of | High | Assigning a shared | +: : Shared Collections Prior : : collection reference and : +: : to In-Place Mutation : : immediately using an : +: : : : in-place operator, : +: : : : unknowingly mutating the : +: : : : original source. : +| **T4-05** | Immutable NamedTuples for | High | Overriding `__hash__` and | +: : Hashable Object : : `__eq__` on a standard : +: : Deduplication : : mutable Python class to : +: : : : allow set insertion. : +| **T4-06** | Strict Empty Set | Critical | Falling back to the empty | +: : Initialization Over : : bracket literal when a : +: : Dictionary Literals : : null set is encountered : +: : : : prior to a union : +: : : : operation. : +| **T4-07** | Immutable Manifest | High | Implementing custom | +: : Elements for Reliable : : `__eq__` and `__hash__` : +: : Deduplication : : on mutable classes and : +: : : : using `list.append()` : +: : : : which allows duplicates : +: : : : to accrue. : +| **T4-08** | Mutually Exclusive XML | Medium | Independent `if` | +: : Node Parsing : : conditions checking : +: : : : `nodeName` sequentially : +: : : : in a loop. : +| **T4-09** | JSON Serialization for | High | Dumping file paths | +: : File Path State Tracking : : sequentially into a flat : +: : : : text file using newlines : +: : : : as the only delimiter. : +| **T4-10** | Set Operations for File | Medium | Using manual for-loops | +: : Deltas : : and list appends to : +: : : : calculate the difference : +: : : : between two collections. : +| **T4-11** | Upstream Base Revision | Medium | Overriding a project | +: : Validation in Layered : : revision without : +: : Manifests : : asserting the previous : +: : : : expected state. : + +-------------------------------------------------------------------------------- + +### Rules + +#### T4-01: Strict Traversal Prefix Matching for Relative URLs + +> **Rule:** Always explicitly evaluate exact path-traversal prefixes (`./` or +> `../`) when resolving relative URLs. Never rely on generic substring matching. +> +> **What:** Relative URL path detection must explicitly evaluate exact +> path-traversal prefixes to avoid inadvertently capturing hidden files or +> custom naming structures. +> +> **Applies To:** URL string manipulation and Git submodule path resolution. +> +> **Why:** The path parser attempted to identify relative submodule paths by +> checking if the URL started with a dot (`.`). This excessively broad +> evaluation failed on identically named hidden repositories or custom +> sub-namespaces (e.g., `.foo`), improperly joining them to the base URL. +> Failing to adhere to this typically results in **Invalid Path Resolution**. + +**Trap 1: Using broad substring matching to detect relative traversal +sequences.** + +**Don't:** + +```python +if url.startswith("."): + url = urllib.parse.urljoin("%s/" % self.remote.url, url) +``` + +**Do:** + +```python +if url.startswith("./") or url.startswith("../"): + url = urllib.parse.urljoin("%s/" % self.remote.url, url) +``` + +-------------------------------------------------------------------------------- + +#### T4-02: Extend-Project Inheritance Scoping + +> **Rule:** Must explicitly restrict `extend-project` XML nodes from +> automatically inheriting contextually local execution scopes (`local:` groups) +> from root parents. +> +> **What:** When managing XML manifestation directives, `extend-project` nodes +> must be actively restricted from inheriting contextually local execution +> scopes (`local:` groups). +> +> **Applies To:** XML manifest engine mappings and override properties. +> +> **Why:** Without explicit protection, extended project entities +> unintentionally inherited top-level project local groups, confusing the +> inclusion and exclusion scoping directives that drive synchronization +> boundaries. Failing to adhere to this typically results in **Scope +> Contamination**. + +**Trap 1: Omitting group inheritance constraints for dynamically overridden node +elements.** + +**Don't:** + +* Allow XML logic to process `extend-project` the exact same way it processes + raw `project` roots without negative test coverage. + +**Do:** + +* Implement testing and parsing guards proving that elements processed under + `extend-project` actively strip `local:` group attributes. + +-------------------------------------------------------------------------------- + +#### T4-03: Tuple Immutability in Membership Checks + +> **Rule:** Always use immutable tuples instead of list literals for membership +> tests against constant sequences within hot loops. +> +> **What:** Utilize immutable tuples `()` instead of list literals `[]` when +> performing membership tests (the `in` operator) against constant sequences to +> minimize reallocation overhead and adhere to idiomatic Python. +> +> **Applies To:** XML manifest parsing logic (`manifest_xml.py`) and hot loops +> across the codebase. +> +> **Why:** During recursive node parsing, list literals were instantiated on +> every iteration solely to check if a node name matched fixed schema elements. +> Failing to adhere to this typically results in **Performance Degradation**. + +**Trap 1: Instantiating transient list objects inside loop conditions.** + +**Don't:** + +```python +if node.nodeName in ["include", "project"]: +``` + +**Do:** + +```python +if node.nodeName in ("include", "project"): +``` + +-------------------------------------------------------------------------------- + +#### T4-04: Defensive Copying of Shared Collections Prior to In-Place Mutation + +> **Rule:** Must defensively copy shared parent collections before applying +> in-place update operators (`|=`), strictly handling null-safety during the +> process. +> +> **What:** When propagating inherited collections (e.g., manifest group sets) +> to child nodes, shared parent sets must be defensively copied prior to +> applying in-place union or update operators (`|=`). Null-safety must be +> handled during the copy. +> +> **Applies To:** XML Manifest Parsing (`manifest_xml.py`); hierarchical state +> propagation where child elements inherit and augment parent states. +> +> **Why:** Assigning a parent's group set directly to a child and then using the +> in-place union operator (`|=`) modified the original parent set in memory. +> This caused the child's specific groups to 'leak' into unrelated sibling nodes +> processed subsequently. Failing to adhere to this typically results in **State +> Corruption / Group Leakage**. + +**Trap 1: Assigning a shared collection reference and immediately using an +in-place operator, unknowingly mutating the original source.** + +**Don't:** + +```python +# BAD: In-place modification mutates the shared parent_groups object +nodeGroups = parent_groups +if node.hasAttribute("groups"): + nodeGroups |= self._ParseSet(node.getAttribute("groups")) +``` + +**Do:** + +```python +# GOOD: Create a localized copy before mutation +nodeGroups = parent_groups.copy() if parent_groups else set() +if node.hasAttribute("groups"): + nodeGroups |= self._ParseSet(node.getAttribute("groups")) +``` + +**Trap 2: Falling back to an empty set instantiation after a copy call without +guarding against a null reference.** + +**Don't:** + +```python +# BAD: Raises AttributeError if parent_groups is None +include_groups = parent_groups.copy() or set() +``` + +**Do:** + +```python +# GOOD: Null-safe default evaluation prior to the copy invocation +include_groups = (parent_groups or set()).copy() +``` + +-------------------------------------------------------------------------------- + +#### T4-05: Immutable NamedTuples for Hashable Object Deduplication + +> **Rule:** Always subclass `NamedTuple` to define custom objects stored in +> hash-based collections. Never implement manual `__hash__` and `__eq__` +> functions on mutable classes. +> +> **What:** Custom objects stored in hash-based collections (like `set` or used +> as `dict` keys) for deduplication must guarantee immutability. Avoid manually +> implementing `__eq__` and `__hash__` on mutable classes; instead, subclass +> `NamedTuple`. +> +> **Applies To:** Manifest deduplication logic (`project.py`); specifically +> elements like `_CopyFile` and `_LinkFile` stored in sets. +> +> **Why:** Manually implementing `__hash__` on a standard class based on its +> mutable `__dict__` attributes created a landmine. If the object's properties +> changed after insertion into a set, the hash mismatched, rendering the object +> unreachable and breaking deduplication. Failing to adhere to this typically +> results in **Memory Leak / Deduplication Failure**. + +**Trap 1: Overriding `__hash__` and `__eq__` on a standard mutable Python class +to allow set insertion.** + +**Don't:** + +```python +# BAD: Properties can mutate, altering the hash post-insertion +class _CopyFile: + def __init__(self, src, dest): + self.src = src + self.dest = dest + def __hash__(self): + return hash(repr(sorted(self.__dict__.items()))) + def __eq__(self, other): + return self.__dict__ == other.__dict__ +``` + +**Do:** + +```python +# GOOD: Use typing.NamedTuple to guarantee immutability +from typing import NamedTuple + +class _CopyFile(NamedTuple): + src: str + dest: str +``` + +-------------------------------------------------------------------------------- + +#### T4-06: Strict Empty Set Initialization Over Dictionary Literals + +> **Rule:** Must explicitly invoke `set()` when initializing empty collections +> meant for set operations. Never fall back to the empty bracket literal `{}`. +> +> **What:** When initializing or providing a fallback empty collection meant for +> set operations (such as union `|` or `|=`), explicitly instantiate a `set()` +> rather than using the empty literal `{}`. +> +> **Applies To:** Python collection management globally; prominently surfaced +> during list-to-set manifest group migrations. +> +> **Why:** During a migration from lists to sets for group parsing, an empty +> default was incorrectly represented as `{}`. Since `{}` evaluates to a `dict` +> in Python, attempting a set union between a valid set and the empty dict threw +> an immediate, fatal `TypeError`. Failing to adhere to this typically results +> in **Runtime Crash / TypeError**. + +**Trap 1: Falling back to the empty bracket literal when a null set is +encountered prior to a union operation.** + +**Don't:** + +```python +# BAD: '{}' creates an empty dictionary, throwing a TypeError upon union +expanded_project_groups = {"all"} | (self.groups or {}) +``` + +**Do:** + +```python +# GOOD: Explicitly initialize an empty set +expanded_project_groups = {"all"} | (self.groups or set()) +``` + +-------------------------------------------------------------------------------- + +#### T4-07: Immutable Manifest Elements for Reliable Deduplication + +> **Rule:** Always represent manifest file operations as inherently immutable +> structures to guarantee safe, order-preserving deduplication. +> +> **What:** Manifest elements representing file operations must be implemented +> as immutable structures (e.g., `NamedTuple`) to ensure safe deduplication and +> deterministic order preservation, avoiding manual string-based hashing. +> +> **Applies To:** Manifest parsing models, specifically `<copyfile>` and +> `<linkfile>` abstractions. +> +> **Why:** Previously, manifest objects were stored in mutable lists, relying on +> manual string representations (`repr`) and sorted dictionary items for +> hashing. This risked state corruption and failed to cleanly deduplicate +> repeated entries while maintaining the relative declaration sequence. Failing +> to adhere to this typically results in **Silent Data Corruption / Redundant +> File Operations**. + +**Trap 1: Implementing custom `__eq__` and `__hash__` on mutable classes and +using `list.append()` which allows duplicates to accrue.** + +**Don't:** + +```python +class _CopyFile: + def __hash__(self): + # BAD: Manual serialization of mutable dictionary + return hash(repr(sorted(self.__dict__.items()))) + +self.copyfiles.append(_CopyFile(src, dest)) +``` + +**Do:** + +```python +class _CopyFile(NamedTuple): + # GOOD: Inherently immutable and hashable + src: str + dest: str + +# GOOD: Using a dict to deduplicate while preserving insertion order +self.copyfiles[_CopyFile(src, dest)] = True +``` + +-------------------------------------------------------------------------------- + +#### T4-08: Mutually Exclusive XML Node Parsing + +> **Rule:** Always chain mutually exclusive XML node type checks using an +> `if-elif` structure rather than independent sequential statements. +> +> **What:** When parsing mutually exclusive XML node types, use an `if-elif` +> structure to avoid redundant condition evaluations. +> +> **Applies To:** XML parsing loops (e.g., iterating over `node.childNodes` in +> `manifest_xml.py`). +> +> **Why:** A sequence of independent `if` statements forced unnecessary string +> comparisons even after a matching node name was found. Failing to adhere to +> this typically results in **CPU Overhead / Inefficient Parsing**. + +**Trap 1: Independent `if` conditions checking `nodeName` sequentially in a +loop.** + +**Don't:** + +```python +if n.nodeName == "copyfile": + self._ParseCopyFile(p, n) +if n.nodeName == "linkfile": + self._ParseLinkFile(p, n) +``` + +**Do:** + +```python +if n.nodeName == "copyfile": + self._ParseCopyFile(p, n) +elif n.nodeName == "linkfile": + self._ParseLinkFile(p, n) +``` + +-------------------------------------------------------------------------------- + +#### T4-09: JSON Serialization for File Path State Tracking + +> **Rule:** Must persist dynamically generated manifest file outputs using +> structured JSON serialization. Never use newline-delimited plaintext for path +> lists. +> +> **What:** Persistent tracking mechanisms for manifest-generated file +> operations must use structured JSON serialization instead of newline-delimited +> plaintext to support special characters in paths. +> +> **Applies To:** Manifest state management (`subcmds/sync.py`), internal +> `.repo` tracking files (e.g., `copy-link-files.json`). +> +> **Why:** Tracking `<copyfile>` and `<linkfile>` operations via flat, +> newline-delimited text files broke serialization and cleanup when file paths +> legally contained newline (`\n`) characters. Structured JSON protects against +> these bounds issues. Failing to adhere to this typically results in **Parsing +> Errors / Orphaned Files**. + +**Trap 1: Dumping file paths sequentially into a flat text file using newlines +as the only delimiter.** + +**Don't:** + +```python +with open('linkfile.list', 'w') as fd: + for path in new_linkfiles: + fd.write(path + '\n') +``` + +**Do:** + +```python +with open('copy-link-files.json', 'w') as fd: + json.dump({'linkfile': new_linkfiles, 'copyfile': new_copyfiles}, fd) +``` + +-------------------------------------------------------------------------------- + +#### T4-10: Set Operations for File Deltas + +> **Rule:** Always compute the delta between unordered lists of file paths using +> native Python set operations (`-`). +> +> **What:** When computing the delta between lists of file paths (e.g., figuring +> out which obsolete files to remove), utilize Python set operations rather than +> manual iterations with truthiness and membership checks. +> +> **Applies To:** Deduplicating objects, comparing manifest states, computing +> file differences. +> +> **Why:** Calculating the difference between historical tracking data and new +> target paths was done using O(N*M) nested iterations with redundant truthiness +> checks, leading to verbose and less performant code. Failing to adhere to this +> typically results in **Algorithmic Inefficiency**. + +**Trap 1: Using manual for-loops and list appends to calculate the difference +between two collections.** + +**Don't:** + +```python +need_remove_files = [] +for old_path in old_dict.get('linkfile', []): + if old_path and old_path not in new_paths: + need_remove_files.append(old_path) +``` + +**Do:** + +```python +need_remove_files = [] +need_remove_files.extend( + set(old_dict.get('linkfile', [])) - + set(new_paths) +) +``` + +**Exceptions:** Ordered constraints where file evaluation sequence matters (sets +are unordered). + +-------------------------------------------------------------------------------- + +#### T4-11: Upstream Base Revision Validation in Layered Manifests + +> **Rule:** Must assert multi-layer manifest overrides (`extend-project`) +> against the parent project's expected state by utilizing a `base-rev` +> attribute. +> +> **What:** When overriding (`extend-project`) or removing (`remove-project`) +> projects in a layered manifest structure, changes should be validated against +> the expected state of the parent project via a `base-rev` attribute. +> +> **Applies To:** XML Manifest parsing logic (`manifest_xml.py`) during +> multi-manifest ingestion. +> +> **Why:** Downstream projects using layered manifests would unknowingly apply +> overrides (like custom revisions) to upstream components that had drastically +> changed their underlying state or branches, resulting in 'undead patches' that +> silently masked critical upstream changes. Failing to adhere to this typically +> results in **Silent Configuration Divergence**. + +**Trap 1: Overriding a project revision without asserting the previous expected +state.** + +**Don't:** + +```xml +<!-- BAD: Blind override leaves system vulnerable to upstream changes --> +<extend-project name="my-project" revision="refs/changes/123" /> +``` + +**Do:** + +```xml +<!-- GOOD: Base-rev acts as a guardrail --> +<extend-project name="my-project" base-rev="upstream_hash" revision="refs/changes/123" /> +``` + +**Trap 2: Checking string-based manifest attributes using implicit truthiness.** + +**Don't:** + +```python +# BAD: Implicit truthiness skips validation if revisionExpr is unexpectedly empty +if base and p.revisionExpr and p.revisionExpr != base: + raise Error() +``` + +**Do:** + +```python +# GOOD: Strict comparison catches empty/None states if a base is declared +if base and p.revisionExpr != base: + raise Error() +``` + +**Exceptions:** The `base-rev` attribute is optional to maintain backward +compatibility for users migrating to the new manifest schema. + +-------------------------------------------------------------------------------- + +### Cross-Domain Dependencies + +* **Upstream:** T2 | Filesystem Atomicity & Worktree Layout - *File path + extraction and submodule path resolution mapped here strictly govern + worktree creation and legacy migration algorithms downstream.* +* **Downstream:** T6 | CLI Argument Parsing & UX Consistency - *Structured + JSON serialization outputted from manifest paths sets the direct format + constraint for CLI UX and tooling integrations.* + +## Chapter: Hermetic Testing & Test Modernization + +**Context:** This domain governs the migration of legacy `unittest` suites to +modern `pytest` functional paradigms and the establishment of hermetic session +fixtures. It enforces strict environment isolation to prevent global state +pollution (e.g., developer `.gitconfig` bleeding) while safely intercepting +standard streams and filesystem paths. + +### Summary + +| Rule ID | Principle / Constraint | Priority | Primary Symptom / | +: : : : Trap : +| :-------- | :------------------------------ | :------- | :------------------ | +| **T5-01** | Idiomatic Truthiness Validation | Medium | Asserting strict | +: : in Tests : : identity against : +: : : : Python boolean : +: : : : singletons. : +| **T5-02** | Pytest Over Legacy Unittest | High | Subclassing | +: : Framework : : `unittest.TestCase` : +: : : : and configuring : +: : : : state inside a : +: : : : `setUp` method for : +: : : : new test files. : +| **T5-03** | Dedicated Test Utilities for | Medium | Copy-pasting | +: : API Helpers : : utility functions : +: : : : across test modules : +: : : : or placing standard : +: : : : functions into : +: : : : pytest conftest : +: : : : files. : +| **T5-04** | Test Framework Modernization to | High | Modifying | +: : Pytest : : environment : +: : : : variables globally : +: : : : at module load to : +: : : : support tests. : +| **T5-05** | Standard Output Capture via | Medium | Using the mock | +: : Context Managers : : framework to : +: : : : intercept a core : +: : : : language stream. : +| **T5-06** | Direct Helper Functions Over | Medium | Defining a pytest | +: : Fixture Factories : : fixture that yields : +: : : : a nested function : +: : : : to bypass global : +: : : : state limitations. : +| **T5-07** | Elimination of Redundant Path | Medium | Chaining explicit | +: : Assertions : : existence checks : +: : : : before access : +: : : : checks. : +| **T5-08** | Pathlib Utility Constants for | Medium | Using nested | +: : Test Path Resolution : : `os.path.dirname` : +: : : : and `os.path.join` : +: : : : to traverse back to : +: : : : the parent : +: : : : directory. : +| **T5-09** | Pytest Adoption for Test Suite | High | Creating test case | +: : Definition : : classes that : +: : : : inherit from : +: : : : `unittest.TestCase` : +: : : : and asserting via : +: : : : `self.assertEqual`. : +| **T5-10** | Minimal Mocking Isolation for | High | Using stacked patch | +: : Test Reliability : : decorators for : +: : : : non-relevant : +: : : : sub-routines (e.g., : +: : : : standard error : +: : : : outputs or : +: : : : colorizers) just to : +: : : : trigger an embedded : +: : : : autocorrection : +: : : : flow. : +| **T5-11** | Canonical Resolution for System | Medium | Patching | +: : Library Mocks : : `time.sleep` by : +: : : : referencing the : +: : : : specific host : +: : : : application file : +: : : : where it was : +: : : : locally imported. : +| **T5-12** | Memoized Function Cache | High | Mocking the | +: : Clearing in Setup : : underlying behavior : +: : : : of a cached : +: : : : function but : +: : : : leaving the LRU : +: : : : cache intact across : +: : : : tests. : +| **T5-13** | Idiomatic Standard Stream | Medium | Using `mock.patch` | +: : Redirection : : to manually inject : +: : : : a `StringIO` object : +: : : : over `sys.stderr`. : +| **T5-14** | Context Manager Nesting | Medium | Adding a new level | +: : Consolidation : : of indentation for : +: : : : every context : +: : : : manager invoked. : +| **T5-15** | Anti-Change Detector Tests | Medium | Asserting exact | +: : : : equality of string : +: : : : outputs including : +: : : : standardized : +: : : : prefixes. : +| **T5-16** | Infrastructure Prebuilts via | Medium | Relying on tools | +: : vpython : : that dynamically : +: : : : build Python from : +: : : : source to test : +: : : : across multiple : +: : : : versions. : +| **T5-17** | Modernized Filesystem Paths via | Medium | Using | +: : Pathlib : : `os.path.join` and : +: : : : `with open(..., : +: : : : 'w')` for basic : +: : : : text file creation : +: : : : in tests. : +| **T5-18** | Standardized Module-Level Mock | Medium | Importing specific | +: : Imports : : classes or : +: : : : exceptions directly : +: : : : from test targets : +: : : : or standard : +: : : : libraries. : +| **T5-19** | Pytest Adoption for New Test | High | Creating subclasses | +: : Modules : : of : +: : : : `unittest.TestCase` : +: : : : and using : +: : : : `self.assertEqual`. : +| **T5-20** | High-Fidelity Test Targets over | High | Using `MagicMock` | +: : Permissive Mocks : : to bypass : +: : : : instantiating core : +: : : : models. : +| **T5-21** | Session-Scoped Hermetic Git | Medium | Executing `git | +: : Identity Injection : : config` inside : +: : : : individual test : +: : : : setup blocks to : +: : : : fake a user : +: : : : identity. : +| **T5-22** | Automated Lifecycle Management | High | Spawning detached | +: : for Test Filesystem Artifacts : : temporary : +: : : : directories and : +: : : : manually : +: : : : maintaining cleanup : +: : : : blocks. : +| **T5-23** | Idiomatic String Evaluation in | Medium | Iterating over raw | +: : Mocked Test Streams : : string splits and : +: : : : manually validating : +: : : : line length to : +: : : : count valid output : +: : : : strings. : +| **T5-24** | Hermetic Isolation of Git | High | Relying on default | +: : Configuration Directories in : : system paths for : +: : Tests : : the home directory : +: : : : during test : +: : : : execution. : +| **T5-25** | Scope Hierarchy Enforcement in | Medium | Injecting | +: : Pytest Fixtures : : `tmp_path` into a : +: : : : fixture decorated : +: : : : with : +: : : : `scope="session"`. : +| **T5-26** | Strict Python Version | Medium | Adding modern | +: : Compatibility for Third-Party : : third-party test : +: : Plugins : : dependencies to : +: : : : legacy-supported : +: : : : projects without : +: : : : validating the : +: : : : minimum required : +: : : : Python constraints. : +| **T5-27** | Hermetic Isolation of Terminal | High | Relying on the | +: : UI Color Configurations : : default state of : +: : : : the `Coloring` : +: : : : class without : +: : : : forcing it during : +: : : : setup, implicitly : +: : : : depending on : +: : : : environmental : +: : : : variables. : +| **T5-28** | Abstract Intent Documentation | Medium | Documenting exact | +: : in Unit Tests : : data values being : +: : : : asserted directly : +: : : : in the test : +: : : : docstring. : + +-------------------------------------------------------------------------------- + +### Rules + +#### T5-01: Idiomatic Truthiness Validation in Tests + +> **Rule:** Always use implicit truthiness or falsiness evaluations in test +> assertions unless strict type matching is explicitly required. +> +> **What:** Test suites must use implicit truthy/falsy validations rather than +> asserting strict identity against boolean singletons unless exact type +> enforcement is necessary. +> +> **Applies To:** Python assertions in test frameworks (pytest, unittest). +> +> **Why:** Tests relying on strict boolean identity (`is True`) became brittle +> when refactoring configurations or flag implementations that returned +> alternative truthy values. Failing to adhere to this typically results in +> **Brittle Assertions**. + +**Trap 1: Asserting strict identity against Python boolean singletons.** + +**Don't:** + +```python +assert opts.include_summary is True +``` + +**Do:** + +```python +assert opts.include_summary +``` + +**Exceptions:** Situations where distinguishing between None and False is +structurally critical to the application logic. + +-------------------------------------------------------------------------------- + +#### T5-02: Pytest Over Legacy Unittest Framework + +> **Rule:** Must implement new test suites using modular `pytest` definitions +> instead of propagating `unittest.TestCase` boilerplate. +> +> **What:** New test modules must be implemented using standalone `pytest` +> functional constructs rather than inheriting from `unittest.TestCase` to avoid +> heavy class-based boilerplate. +> +> **Applies To:** Test module architecture (`tests/test_*.py`). +> +> **Why:** The project previously utilized deeply nested `unittest.TestCase` +> structures with complex `setUp` inheritance, which obfuscated test +> initialization and hindered code reusability. Failing to adhere to this +> typically results in **High Boilerplate / Lock-In**. + +**Trap 1: Subclassing `unittest.TestCase` and configuring state inside a `setUp` +method for new test files.** + +**Don't:** + +```python +class InfoCommand(unittest.TestCase): + def setUp(self): + self.cmd = build_cmd() +``` + +**Do:** + +```python +def _get_cmd(): + return build_cmd() + +def test_command_defaults(): + cmd = _get_cmd() + assert cmd.is_valid() +``` + +-------------------------------------------------------------------------------- + +#### T5-03: Dedicated Test Utilities for API Helpers + +> **Rule:** Never overload `conftest.py` with generic API helpers; always place +> reusable test queries in dedicated utility modules. +> +> **What:** Shared test APIs and environment-probing logic (e.g., checking Git +> feature support) must be placed in dedicated utility modules (like +> `utils_for_test.py`) rather than duplicated or shoved into `conftest.py`. +> +> **Applies To:** Test suite architecture and environment scaffolding. +> +> **Why:** Developers were duplicating identical environment checks across +> different test files or misusing `conftest.py` (which should only contain +> fixtures) to store functional APIs. Failing to adhere to this typically +> results in **Test Code Duplication**. + +**Trap 1: Copy-pasting utility functions across test modules or placing standard +functions into pytest conftest files.** + +**Don't:** + +```python +# Inside test_project.py AND test_git_refs.py +def _SupportsReftable(): + return check_git_support() +``` + +**Do:** + +```python +# Inside tests/utils_for_test.py +def supports_reftable(): + return check_git_support() + +# Inside test_project.py +import utils_for_test +@unittest.skipUnless(utils_for_test.supports_reftable(), "...") +``` + +**Exceptions:** Test fixtures (such as initialized directories or mock +generators) which belong in `conftest.py`. + +-------------------------------------------------------------------------------- + +#### T5-04: Test Framework Modernization to Pytest + +> **Rule:** Avoid global module-level environment mutations; consistently +> leverage `pytest` global configuration fixtures. +> +> **What:** Legacy `unittest.TestCase` structures must be migrated to functional +> `pytest` paradigms, leveraging global configuration fixtures over class-based +> `setUp` and `tearDown` methods. +> +> **Applies To:** Entire Python test suite and all new unit tests. +> +> **Why:** Heavy inheritance and isolated setup code within `unittest.TestCase` +> resulted in repeated environment mutation logic, obscured dependencies, and an +> inability to easily share global setup mechanisms like dummy user identities +> or tracing control. Failing to adhere to this typically results in **Global +> State Pollution**. + +**Trap 1: Modifying environment variables globally at module load to support +tests.** + +**Don't:** + +```python +import os +os.environ['REPO_TRACE'] = '0' + +class GitRefsTest(unittest.TestCase): + def setUp(self): + self.tempdir = tempfile.TemporaryDirectory() +``` + +**Do:** + +```python +# Rely on conftest.py fixtures automatically handling environments. +def test_git_refs(tmp_path, disable_repo_trace, setup_user_identity): + gitdir = tmp_path / '.git' + # Run assertions using standard assert statements + assert head == refs.get('HEAD') +``` + +-------------------------------------------------------------------------------- + +#### T5-05: Standard Output Capture via Context Managers + +> **Rule:** Must utilize `contextlib.redirect_stdout` to capture standard output +> streams rather than injecting raw stream mocks. +> +> **What:** Use the standard library `contextlib.redirect_stdout` for capturing +> command output in test scopes, instantiated directly as a one-liner, rather +> than utilizing the `unittest.mock` framework to patch system streams. +> +> **Applies To:** Python test suites (Pytest/Unittest) intercepting CLI stdout +> payloads. +> +> **Why:** Tests were previously relying on `@mock.patch` for `sys.stdout` or +> utilizing multi-line context manager instantiations, which unnecessarily +> coupled output capture to the mocking engine and added boilerplate. Failing to +> adhere to this typically results in **Unnecessary Mocking / Test Bloat**. + +**Trap 1: Using the mock framework to intercept a core language stream.** + +**Don't:** + +```python +@mock.patch("sys.stdout", new_callable=io.StringIO) +def test_something(self, mock_stdout): + # test logic +``` + +**Do:** + +```python +import contextlib +import io + +def test_something(): + with contextlib.redirect_stdout(io.StringIO()) as stdout: + # test logic +``` + +**Trap 2: Creating the string buffer variable outside the context manager scope +unnecessarily.** + +**Don't:** + +```python +stdout = io.StringIO() +with contextlib.redirect_stdout(stdout): + _run_status(manifest, ["-o"]) +``` + +**Do:** + +```python +with contextlib.redirect_stdout(io.StringIO()) as stdout: + _run_status(manifest, ["-o"]) +``` + +-------------------------------------------------------------------------------- + +#### T5-06: Direct Helper Functions Over Fixture Factories + +> **Rule:** Avoid defining dummy `pytest` fixtures solely to return nested +> function callbacks; use straightforward module-level helpers instead. +> +> **What:** Do not abuse the Pytest fixture system to generate and return local +> functions. If a utility does not benefit from the fixture lifecycle management +> (setup/teardown), it should be a standard module-level or private helper +> function. +> +> **Applies To:** Pytest files requiring test data initialization or repetitive +> setup procedures. +> +> **Why:** Test frameworks became overly complex when developers wrapped simple +> initialization logic in `@pytest.fixture` annotations just to yield a callable +> function to the test case. Failing to adhere to this typically results in +> **Fixture Graph Bloat**. + +**Trap 1: Defining a pytest fixture that yields a nested function to bypass +global state limitations.** + +**Don't:** + +```python +@pytest.fixture +def init_temp_git_tree(): + def _init_temp_git_tree(git_dir: Path) -> None: + # init logic + return _init_temp_git_tree +``` + +**Do:** + +```python +def _init_temp_git_tree(git_dir: Path) -> None: + # init logic +``` + +-------------------------------------------------------------------------------- + +#### T5-07: Elimination of Redundant Path Assertions + +> **Rule:** Never prepend path existence assertions prior to invoking +> permissions or loading assertions that implicitly validate existence. +> +> **What:** Do not assert the existence of a file prior to asserting its +> specific permissions or state. Subsequent OS-level operations implicitly +> validate path existence. +> +> **Applies To:** Python unittests validating filesystem layouts, binary +> configurations, or hook executable permissions. +> +> **Why:** Tests contained boilerplate blocks invoking `os.path.isfile()` purely +> to offer a custom string format failure, even though the immediate next line +> invoking `os.access()` would fail correctly with an implicit lack of +> existence. Failing to adhere to this typically results in **Test +> Boilerplate**. + +**Trap 1: Chaining explicit existence checks before access checks.** + +**Don't:** + +```python +self.assertTrue(os.path.isfile(repo_path), f"{repo_path} does not exist") +self.assertTrue(os.access(repo_path, os.X_OK), f"{repo_path} is not executable") +``` + +**Do:** + +```python +self.assertTrue(os.access(repo_path, os.X_OK), f"{repo_path} is not executable") +``` + +-------------------------------------------------------------------------------- + +#### T5-08: Pathlib Utility Constants for Test Path Resolution + +> **Rule:** Always resolve test filesystem boundaries using centralized +> `pathlib` objects instead of string-concatenating legacy `os.path` operations. +> +> **What:** Path constructions in test files should utilize modern pathlib +> objects via centralized utility constants (e.g., `utils_for_test.THIS_DIR`) +> instead of verbose `os.path` operations. +> +> **Applies To:** Python test suites interacting with the file system. +> +> **Why:** Manual path constructions using nested `os.path.dirname(__file__)` +> calls created verbose, hard-to-read boilerplate that cluttered the test logic. +> Failing to adhere to this typically results in **Verbose Code Base**. + +**Trap 1: Using nested `os.path.dirname` and `os.path.join` to traverse back to +the parent directory.** + +**Don't:** + +```python +repo_path = os.path.join(os.path.dirname(os.path.dirname(__file__)), "repo") +``` + +**Do:** + +```python +repo_path = utils_for_test.THIS_DIR.parent / "repo" +``` + +-------------------------------------------------------------------------------- + +#### T5-09: Pytest Adoption for Test Suite Definition + +> **Rule:** Must author all file-level test assertions utilizing `pytest` syntax +> constructs over legacy `unittest` equivalents. +> +> **What:** Test files must be defined using the modern `pytest` framework, +> avoiding class-based `unittest.TestCase` inheritance and relying on standard +> `assert` semantics. +> +> **Applies To:** The entire Python testing suite. +> +> **Why:** The legacy `unittest` structure required extensive, verbose +> boilerplate for state setup, test case classes, and specific assertion methods +> that restricted readability. Failing to adhere to this typically results in +> **Test Verbosity**. + +**Trap 1: Creating test case classes that inherit from `unittest.TestCase` and +asserting via `self.assertEqual`.** + +**Don't:** + +```python +import unittest +class RepoTests(unittest.TestCase): + def test_example(self): + self.assertEqual(a, b) +``` + +**Do:** + +```python +# Standalone pytest functions +def test_example(): + assert a == b +``` + +-------------------------------------------------------------------------------- + +#### T5-10: Minimal Mocking Isolation for Test Reliability + +> **Rule:** Avoid deeply nested or exhaustively mocked test wrappers; isolate +> and invoke inner target logic primitives instead. +> +> **What:** Mock usage must be limited strictly to necessary external +> intercepts. Exhaustive mock environments must not be created just to drive a +> small piece of inner logic; instead, structural refactoring must isolate the +> logical boundary. +> +> **Applies To:** Test suites involving deep API structures and side-effects. +> +> **Why:** Relying on heavily nested, brittle 'house of cards' mock +> configurations meant tests frequently failed when unrelated code modifications +> broke the strict internal mock assumptions, turning tests into throwaway +> artifacts. Failing to adhere to this typically results in **Fragile Mock +> Collapse**. + +**Trap 1: Using stacked patch decorators for non-relevant sub-routines (e.g., +standard error outputs or colorizers) just to trigger an embedded autocorrection +flow.** + +**Don't:** + +```python +with mock.patch("main.RepoClient", return_value=mock_client): + with mock.patch("main.SetDefaultColoring"): + repo._RunLong("tart", gopts, argv, log) +``` + +**Do:** + +* Isolate the logic into `_autocorrect_command_name()` and test it by passing + minimal data primitives: `res = self.repo._autocorrect_command_name("tart", + mock_config)`. + +**Exceptions:** Testing hard, native OS-level boundary bindings. + +-------------------------------------------------------------------------------- + +#### T5-11: Canonical Resolution for System Library Mocks + +> **Rule:** Always patch standard libraries at their canonical origin namespace +> rather than targeting the local module import resolution boundary. +> +> **What:** Standard library functions must be targeted for testing patches at +> their canonical origin namespace rather than the local import footprint of the +> target module. +> +> **Applies To:** Test configuration defining `mock.patch` paths. +> +> **Why:** Patching against the local module import representation strictly +> coupled tests to a given file's exact inclusion syntax. Minor refactors +> altering module import aliases completely disabled testing verification. +> Failing to adhere to this typically results in **Brittle Patch Routing**. + +**Trap 1: Patching `time.sleep` by referencing the specific host application +file where it was locally imported.** + +**Don't:** + +```python +@mock.patch("main.time.sleep") +``` + +**Do:** + +```python +@mock.patch("time.sleep") +``` + +-------------------------------------------------------------------------------- + +#### T5-12: Memoized Function Cache Clearing in Setup + +> **Rule:** Always flush the `lru_cache` of mocked globally-accessible memoized +> functions within the test initialization sequence. +> +> **What:** When mocking globally accessible functions decorated with +> `@functools.lru_cache`, the cache must be explicitly cleared in the test +> suite's `setUp` routine. +> +> **Applies To:** Unit test suites interacting with memoized functions. +> +> **Why:** Failing to clear the `lru_cache` of globally executed functions +> during testing caused subsequent tests in the runner to receive cached mock +> results injected by prior tests, leading to non-deterministic, cascading +> failures. Failing to adhere to this typically results in **State Contamination +> / Flaky Tests**. + +**Trap 1: Mocking the underlying behavior of a cached function but leaving the +LRU cache intact across tests.** + +**Don't:** + +```python +class SshTests(unittest.TestCase): + def test_version(self): + with mock.patch("ssh._run_ssh_version", return_value="OpenSSH_1.2\n"): + self.assertEqual(ssh.version(), (1, 2)) +``` + +**Do:** + +```python +class SshTests(unittest.TestCase): + def setUp(self): + ssh.version.cache_clear() + + def test_version(self): + with mock.patch("ssh._run_ssh_version", return_value="OpenSSH_1.2\n"): + self.assertEqual(ssh.version(), (1, 2)) +``` + +-------------------------------------------------------------------------------- + +#### T5-13: Idiomatic Standard Stream Redirection + +> **Rule:** Always employ `contextlib.redirect_stderr` directly instead of +> overwriting standard stream objects globally using patch frameworks. +> +> **What:** To intercept `stderr` or `stdout` in testing, utilize the standard +> library's context managers (`contextlib.redirect_stderr`) instead of +> overriding the stream objects globally using `mock.patch`. +> +> **Applies To:** Unit testing standard output logs and error streams. +> +> **Why:** Using generic mocks to replace `sys.stderr` creates brittle tests +> with broader side effects. The standard library provides dedicated tools +> strictly designed for capturing stream text safely. Failing to adhere to this +> typically results in **Non-Idiomatic Tooling / Brittle Mocks**. + +**Trap 1: Using `mock.patch` to manually inject a `StringIO` object over +`sys.stderr`.** + +**Don't:** + +```python +with mock.patch("sys.stderr", new=io.StringIO()) as mock_stderr: + run_command() +``` + +**Do:** + +```python +f = io.StringIO() +with contextlib.redirect_stderr(f): + run_command() +``` + +-------------------------------------------------------------------------------- + +#### T5-14: Context Manager Nesting Consolidation + +> **Rule:** Consolidate contiguous mock or context manager deployments into +> single-line definitions to minimize indentation footprints. +> +> **What:** Contiguous, deeply nested context managers (`with` blocks) should be +> consolidated into a single `with` statement to reduce horizontal indentation +> and structural complexity. +> +> **Applies To:** Complex test fixtures or code paths requiring multiple +> synchronized states or file handles. +> +> **Why:** Individual context managers per line resulted in excessive code +> indentation (pyramid of doom), harming readability and consuming unnecessary +> screen space. Failing to adhere to this typically results in **Excessive +> Indentation / Pyramid of Doom**. + +**Trap 1: Adding a new level of indentation for every context manager invoked.** + +**Don't:** + +```python +with mock.patch("sys.stderr") as mock_stderr: + with mock.patch("socket.socket") as ms: + run_test() +``` + +**Do:** + +```python +with mock.patch("sys.stderr") as mock_stderr, mock.patch("socket.socket") as ms: + run_test() +``` + +-------------------------------------------------------------------------------- + +#### T5-15: Anti-Change Detector Tests + +> **Rule:** Avoid exact full-string assertions when evaluating formatted logs or +> stringified error conditions to prevent brittle side-effect maintenance. +> +> **What:** When verifying error logging or outputs, tests must assert on +> flexible core substrings rather than exact line matches, and exceptions +> configured as mock side effects should use class types rather than +> instantiated string objects. +> +> **Applies To:** Unit testing string outputs, logs, and simulated mock +> failures. +> +> **Why:** Tests acting as 'change detectors' failed unnecessarily when minor +> formatting, capitalization, or prefix strings (e.g., 'WARNING' vs 'warning') +> were updated in the underlying code, despite the core logic remaining +> functional. Failing to adhere to this typically results in **Brittle Tests / +> Extraneous Boilerplate**. + +**Trap 1: Asserting exact equality of string outputs including standardized +prefixes.** + +**Don't:** + +```python +self.assertEqual(stderr.getvalue(), "repo: warning: git trace2 logging failed: Mock error") +``` + +**Do:** + +```python +self.assertIn("git trace2 logging failed", stderr.getvalue()) +``` + +**Trap 2: Instantiating an exception with a dummy string for a simple +side-effect mock.** + +**Don't:** + +```python +mock_socket.connect.side_effect = OSError("Mock error") +``` + +**Do:** + +```python +mock.patch("socket.socket", side_effect=OSError) +``` + +**Exceptions:** Tests explicitly verifying exact formatting serialization (e.g., +JSON schema adherence). + +-------------------------------------------------------------------------------- + +#### T5-16: Infrastructure Prebuilts via vpython + +> **Rule:** Must execute continuous integration environments using `vpython` +> wrappers instead of relying on legacy source-compiling orchestration +> frameworks like `pyenv`. +> +> **What:** Leverage `vpython` for multi-version Python testing rather than +> `tox` combined with `pyenv` to utilize prebuilt binaries and minimize +> environment setup overhead. +> +> **Applies To:** Hermetic testing setups, CI pipelines, and multi-version +> coverage scripts. +> +> **Why:** Using `pyenv` and `tox` forced CI and local test runs to download and +> compile Python versions from source on the fly, adding minutes of overhead to +> every test run. Shifting to `vpython` leveraged prebuilt infrastructure +> runtimes. Failing to adhere to this typically results in **Slow CI Builds**. + +**Trap 1: Relying on tools that dynamically build Python from source to test +across multiple versions.** + +**Don't:** + +* Using `tox.ini` with `pyenv` to orchestrate matrix testing across Python + versions, incurring source-compilation penalties. + +**Do:** + +* Invoking tests via `vpython`, which provisions hermetic virtual environments + rapidly using precompiled infrastructure binaries. + +-------------------------------------------------------------------------------- + +#### T5-17: Modernized Filesystem Paths via Pathlib + +> **Rule:** Always employ `pathlib` objects for mock file I/O operations rather +> than manual string handling and context managers. +> +> **What:** Test case file system operations should use the modern `pathlib` +> library rather than legacy `os.path` and manual file handles. +> +> **Applies To:** Unit tests and file system operations across the codebase. +> +> **Why:** Legacy tests relied heavily on `os.path.join` and `with open(...)` +> which led to verbose, repetitive boilerplate when constructing mock repository +> states. Failing to adhere to this typically results in **Verbose Boilerplate / +> Maintainability Overhead**. + +**Trap 1: Using `os.path.join` and `with open(..., 'w')` for basic text file +creation in tests.** + +**Don't:** + +```python +# BAD: Verbose manual file writing +root_m = os.path.join(self.manifest_dir, "root.xml") +with open(root_m, "w") as fp: + fp.write("<manifest>...") +``` + +**Do:** + +```python +# GOOD: Pathlib usage for concise writes +root_m = self.manifest_dir / "root.xml" +root_m.write_text("<manifest>...") +``` + +-------------------------------------------------------------------------------- + +#### T5-18: Standardized Module-Level Mock Imports + +> **Rule:** Import external test dependencies and internal domain targets as +> complete module spaces rather than cherry-picking precise classes. +> +> **What:** Test files must import `mock` as a module (`from unittest import +> mock`) and internal targets as a module (`from subcmds import wipe`), rather +> than importing specific classes directly. +> +> **Applies To:** All unit tests. +> +> **Why:** Direct class imports caused namespace pollution and deviated from the +> Google Python Style Guide, making it harder to track the origin of +> dependencies. Failing to adhere to this typically results in **Style Guide +> Violation / Namespace Pollution**. + +**Trap 1: Importing specific classes or exceptions directly from test targets or +standard libraries.** + +**Don't:** + +```python +from unittest.mock import MagicMock +from subcmds.wipe import Wipe, WipeError + +mock_obj = MagicMock() +``` + +**Do:** + +```python +from unittest import mock +from subcmds import wipe + +mock_obj = mock.MagicMock() +``` + +-------------------------------------------------------------------------------- + +#### T5-19: Pytest Adoption for New Test Modules + +> **Rule:** Must restrict the creation of new `unittest.TestCase` files strictly +> unless explicitly bypassing native pytest infrastructure constraints. +> +> **What:** New test modules must be written using the `pytest` framework rather +> than the legacy `unittest` structure. +> +> **Applies To:** All newly added test files. +> +> **Why:** The project was modernizing its testing infrastructure, aiming to +> eliminate `unittest` boilerplate and leverage native `pytest` fixtures for +> global state management. Failing to adhere to this typically results in +> **Technical Debt / Legacy Test Framework Propagation**. + +**Trap 1: Creating subclasses of `unittest.TestCase` and using +`self.assertEqual`.** + +**Don't:** + +```python +class WipeUnitTest(unittest.TestCase): + def test_wipe(self): + self.assertTrue(os.path.exists(path)) +``` + +**Do:** + +```python +def test_wipe(tmp_path): + assert os.path.exists(path) +``` + +**Exceptions:** Unless a compelling reason exists requiring native `unittest` +mechanics. + +-------------------------------------------------------------------------------- + +#### T5-20: High-Fidelity Test Targets over Permissive Mocks + +> **Rule:** Avoid stubbing robust architectural models like `Project` or +> `Manifest` via permissive `MagicMock` allocations; instantiate genuine domain +> classes instead. +> +> **What:** Tests should instantiate and utilize real domain objects (e.g., the +> `Project` class) rather than relying heavily on generic `MagicMock` instances, +> to ensure test fidelity. +> +> **Applies To:** Unit testing, particularly for core models like `Project` or +> `Manifest`. +> +> **Why:** Overuse of `MagicMock` led to tests passing despite broken +> implementations because the mocks lacked side-effects and realistic attribute +> validation. Failing to adhere to this typically results in **False Positive +> Test Results**. + +**Trap 1: Using `MagicMock` to bypass instantiating core models.** + +**Don't:** + +```python +# BAD: Mocks won't fail if the API changes +proj = mock.MagicMock() +proj.name = "test" +proj.DeleteWorktree.side_effect = lambda: True +``` + +**Do:** + +```python +# GOOD: Using actual domain objects for accurate side-effects +proj = Project(manifest=mock_manifest, name="test", ...) +``` + +**Exceptions:** External network calls or complex, deeply decoupled subsystems +where instantiating the real object is prohibitively difficult. + +-------------------------------------------------------------------------------- + +#### T5-21: Session-Scoped Hermetic Git Identity Injection + +> **Rule:** Must inject pseudo Git commit identities as session-scoped +> environment variables rather than manually rewriting local Git configurations +> per test. +> +> **What:** Automated test suites must globally stub Git's author and committer +> identities using session-scoped environment variables to ensure execution in +> clean or unconfigured environments. +> +> **Applies To:** Pytest configuration (`tests/conftest.py`) and all tests +> executing raw Git subprocess commands. +> +> **Why:** Tests interacting with local Git repositories crashed with +> `GitCommandError` on CI runners or clean developer machines lacking a global +> `user.name` and `user.email`. Patching this locally in tests via `git config` +> calls introduced I/O overhead and redundant code. Failing to adhere to this +> typically results in **GitCommandError / Test Flakiness**. + +**Trap 1: Executing `git config` inside individual test setup blocks to fake a +user identity.** + +**Don't:** + +```python +def test_commit(self): + subprocess.run(["git", "config", "user.name", "Foo Bar"]) + subprocess.run(["git", "config", "user.email", "foo@bar.com"]) + # Run test logic +``` + +**Do:** + +```python +@pytest.fixture(autouse=True, scope="session") +def setup_user_identity(monkeysession): + """Set env variables for author and committer name and email.""" + monkeysession.setenv("GIT_AUTHOR_NAME", "Foo Bar") + monkeysession.setenv("GIT_COMMITTER_NAME", "Foo Bar") + monkeysession.setenv("GIT_AUTHOR_EMAIL", "foo@bar.baz") + monkeysession.setenv("GIT_COMMITTER_EMAIL", "foo@bar.baz") +``` + +-------------------------------------------------------------------------------- + +#### T5-22: Automated Lifecycle Management for Test Filesystem Artifacts + +> **Rule:** Must utilize automated `TemporaryDirectory` constructs integrated +> securely into test teardown sequences instead of performing unmanaged manual +> disk sweeps. +> +> **What:** Test fixtures must use state-managed temporary directory constructs +> (e.g., `tempfile.TemporaryDirectory`) whose lifecycle is explicitly bound to +> standard unit test teardown phases, avoiding manual unmanaged path allocation +> and raw removal. +> +> **Applies To:** All test suites performing filesystem mock operations or Git +> repository initialization (`tests/test_subcmds_forall.py`). +> +> **Why:** Tests were manually allocating paths via `tempfile.mkdtemp` and +> issuing raw `shutil.rmtree` calls. This pattern proved fragile, resulting in +> leaked directories across runs or skipped cleanup phases if a test failed +> early. Failing to adhere to this typically results in **Orphaned Artifacts / +> Resource Leaks**. + +**Trap 1: Spawning detached temporary directories and manually maintaining +cleanup blocks.** + +**Don't:** + +```python +def setUp(self): + self.tempdir = tempfile.mkdtemp() + +def tearDown(self): + shutil.rmtree(self.tempdir, ignore_errors=True) +``` + +**Do:** + +```python +def setUp(self): + self.tempdirobj = tempfile.TemporaryDirectory(prefix="forall_tests") + self.tempdir = self.tempdirobj.name + +def tearDown(self): + self.tempdirobj.cleanup() +``` + +-------------------------------------------------------------------------------- + +#### T5-23: Idiomatic String Evaluation in Mocked Test Streams + +> **Rule:** Avoid manually slicing and measuring length properties for string +> evaluations; consistently evaluate standard streams using built-in truthiness +> routines. +> +> **What:** Evaluating outputs from captured stdout or stderr in tests must +> utilize Pythonic abstractions like `splitlines()` and implicit truthiness +> evaluation, rather than raw newline splitting and explicit length checks. +> +> **Applies To:** Hermetic Test suites executing mock command line calls and +> inspecting `sys.stdout`. +> +> **Why:** Legacy test code relied on manual string manipulation like +> `split("\n")` and boolean evaluations like `len(line) > 0`. This approach was +> unidiomatic, fragile against cross-platform line endings, and bloated test +> suite verbosity. Failing to adhere to this typically results in **Fragile +> Tests / Unidiomatic Logic**. + +**Trap 1: Iterating over raw string splits and manually validating line length +to count valid output strings.** + +**Don't:** + +```python +line_count = 0 +for line in mock_stdout.getvalue().split("\n"): + if len(line) > 0: + line_count += 1 +``` + +**Do:** + +```python +line_count = sum(1 if x else 0 for x in mock_stdout.getvalue().splitlines()) +``` + +-------------------------------------------------------------------------------- + +#### T5-24: Hermetic Isolation of Git Configuration Directories in Tests + +> **Rule:** Always isolate Git initialization by injecting temporary dummy +> directories via environment overrides (`HOME` / `USERPROFILE`) to restrict +> test interference. +> +> **What:** Tests utilizing local git operations must hermetically isolate +> environment variables like `HOME` and `USERPROFILE` to prevent developers' +> global configurations from leaking into test executions. +> +> **Applies To:** Global test initialization routines (`conftest.py`, Pytest +> fixtures, class-level test setup). +> +> **Why:** Historically, if a developer's global `.gitconfig` required GPG +> signing (`commit.gpgsign=true`), a git subprocess spawned during test setup +> would unexpectedly open an interactive prompt (e.g., vim) to solicit a signing +> key, causing the test suite to deadlock permanently. Failing to adhere to this +> typically results in **Test Deadlock / State Bleed**. + +**Trap 1: Relying on default system paths for the home directory during test +execution.** + +**Don't:** + +```python +# BAD: Allows test subprocesses to read ~/.gitconfig +subprocess.run(['git', 'tag', '--annotate']) +``` + +**Do:** + +```python +# GOOD: Override environment variables to an isolated temp directory +@pytest.fixture(autouse=True, scope="session") +def alt_home(tmp_path_factory, monkeysession): + var = "USERPROFILE" if platform_utils.isWindows() else "HOME" + monkeysession.setenv(var, str(tmp_path_factory.mktemp("home"))) +``` + +**Exceptions:** Tests that explicitly exist to validate the parsing logic of a +user's real, existing `.gitconfig` (which is highly discouraged). + +-------------------------------------------------------------------------------- + +#### T5-25: Scope Hierarchy Enforcement in Pytest Fixtures + +> **Rule:** Never propagate lower-scoped test fixtures (e.g., function-scoped +> states) into global session-scoped initialization procedures. +> +> **What:** A session-scoped Pytest fixture cannot depend on lower-scoped +> fixtures (such as a function-scoped `tmp_path`). To create temporary paths at +> the session level, `tmp_path_factory.mktemp` must be utilized. +> +> **Applies To:** Pytest fixture definitions, specifically within `conftest.py`. +> +> **Why:** A session-scoped setup mechanism attempted to use the function-scoped +> `tmp_path` fixture. This caused pytest initialization failures. Furthermore, +> reusing function-scoped directories for session-level state caused unintended +> disk thrashing as the directory was continually wiped at the teardown of every +> function. Failing to adhere to this typically results in **Scope Mismatch / +> State Masking**. + +**Trap 1: Injecting `tmp_path` into a fixture decorated with +`scope="session"`.** + +**Don't:** + +```python +# BAD: Session fixture depending on function fixture +@pytest.fixture(scope="session") +def session_tmp_homedir(tmp_path): + return tmp_path +``` + +**Do:** + +```python +# GOOD: Using the factory to generate a session-level path +@pytest.fixture(scope="session") +def session_tmp_homedir(tmp_path_factory): + return tmp_path_factory.mktemp("home") +``` + +-------------------------------------------------------------------------------- + +#### T5-26: Strict Python Version Compatibility for Third-Party Plugins + +> **Rule:** Must strictly align test dependency injection configurations against +> the underlying project's absolute minimum supported Python versions. +> +> **What:** Test tooling dependencies must strictly align with the project's +> minimum supported Python version. External plugin implementation logic should +> be inlined with proper attribution if the plugin requires a newer Python +> version than the project supports. +> +> **Applies To:** Dependency management (`tox.ini`), `conftest.py`, and CI +> environments. +> +> **Why:** A developer introduced a new Pytest plugin to manage isolated home +> directories. However, the plugin required Python 3.8+, while the project +> maintained compatibility down to Python 3.6+. This mismatch caused post-submit +> CI pipeline checks to crash upon test environment installation. Failing to +> adhere to this typically results in **CI Installation Failure**. + +**Trap 1: Adding modern third-party test dependencies to legacy-supported +projects without validating the minimum required Python constraints.** + +**Don't:** + +```ini +# BAD: tox.ini adds dependency unsupported by CI target +deps = + pytest + pytest-home >= 0.4 +``` + +**Do:** + +```python +# GOOD: Inline the logic and remove the external dependency +# adapted from pytest-home 0.5.1 +@pytest.fixture(autouse=True, scope="session") +def session_tmp_homedir(tmp_path_factory, monkeysession): + # implementation logic here +``` + +**Exceptions:** When the project's minimum supported Python version is +officially bumped to match the plugin's requirement. + +-------------------------------------------------------------------------------- + +#### T5-27: Hermetic Isolation of Terminal UI Color Configurations + +> **Rule:** Must aggressively lock default terminal coloring states when +> generating UI tests to isolate regressions from variable environment +> pipelines. +> +> **What:** Unit tests that assert on UI or logging text containing ANSI color +> codes must explicitly force the coloring state to prevent ambient git +> configurations or CI terminal limitations from altering test outcomes. +> +> **Applies To:** Unit tests interacting with the `color` or `repo_logging` +> modules. +> +> **Why:** Tests validating color outputs failed non-deterministically based on +> the ambient environment (e.g., CI runners lacking a TTY, or developers having +> `color.ui=false` in their global `.gitconfig`). Failing to adhere to this +> typically results in **Flaky Tests / Non-Deterministic Failures**. + +**Trap 1: Relying on the default state of the `Coloring` class without forcing +it during setup, implicitly depending on environmental variables.** + +**Don't:** + +```python +def setUp(self): + config_fixture = fixture("test.gitconfig") + self.config = git_config.GitConfig(config_fixture) + self.color = color.Coloring(self.config, "status") +``` + +**Do:** + +```python +def setUp(self): + config_fixture = fixture("test.gitconfig") + self.config = git_config.GitConfig(config_fixture) + color.SetDefaultColoring("always") + self.color = color.Coloring(self.config, "status") +``` + +-------------------------------------------------------------------------------- + +#### T5-28: Abstract Intent Documentation in Unit Tests + +> **Rule:** Never embed exact literal assertions or explicit structural values +> directly inside unit test docstring commentary. +> +> **What:** Test docstrings should describe the behavioral intent or structural +> coverage of the test rather than hardcoding the exact assertion values or +> low-level implementation details. +> +> **Applies To:** Python `unittest` module docstrings. +> +> **Why:** Docstrings that hardcoded exact ANSI escape sequences or config +> values became stale and misleading whenever the underlying mappings or +> formatting codes were updated. Failing to adhere to this typically results in +> **Documentation Rot / Maintenance Overhead**. + +**Trap 1: Documenting exact data values being asserted directly in the test +docstring.** + +**Don't:** + +```python +def test_Color_Parse_first_parameter_none(self): + """fg is black(30), bg is red(31+10=41), attr is ul(4)""" + val = self.color._parse(None, 'black', 'red', 'ul') + self.assertEqual('\x1b[4;30;41m', val) +``` + +**Do:** + +```python +def test_Color_Parse_first_parameter_none(self): + """check fg & bg & attr""" + val = self.color._parse(None, "black", "red", "ul") + self.assertEqual("\x1b[4;30;41m", val) +``` + +-------------------------------------------------------------------------------- + +### Cross-Domain Dependencies + +* **Upstream:** T2 | Filesystem Atomicity & Worktree Layout - *Managing safe + creation and atomic removal of temporary worktrees and `.git` directories + supports reliable test state fixtures.* +* **Downstream:** T3 | Subprocess Git Integration & Error Translation - + *Hermetic test constraints directly dictate how standard git streams, + configuration injections, and subprocess executions are mocked or + intercepted.* +* **Downstream:** T6 | CLI Argument Parsing & UX Consistency - *Strict + terminal environment control in testing enforces deterministic verification + of CLI autocorrect flows and standardized UI colorizations.* + +## Chapter: CLI Argument Parsing & UX Consistency + +**Context:** This chapter governs the lifecycle, validation, and execution of +command-line arguments, enforcing strict standardization for machine-readable +serialization, unified logging, and deterministic, thread-safe terminal +interactions. + +### Summary + +| Rule ID | Principle / | Priority | Primary Symptom / | +: : Constraint : : Trap : +| :-------- | :--------------- | :------- | :-------------------- | +| **T6-01** | Deterministic | High | Providing only | +: : Machine-Readable : : aesthetically : +: : Output Formats : : formatted terminal : +: : : : text for diagnostic : +: : : : commands. : +| **T6-02** | Elimination of | Medium | Assigning a short | +: : Niche Short : : option to a feature : +: : Options : : that isn't commonly : +: : : : used by developers. : +| **T6-03** | Extensible | Medium | Creating rigid, | +: : Boolean Toggle : : single-purpose : +: : Flags : : boolean switches for : +: : : : granular output : +: : : : components. : +| **T6-04** | Context-Aware | Medium | Hardcoding visual | +: : CLI Output : : formatting elements : +: : Separators : : directly after a : +: : : : primary block instead : +: : : : of conditionally : +: : : : checking the next : +: : : : block. : +| **T6-05** | Output Format | Medium | Handling output | +: : Standardization : : formatting via raw : +: : via Enums : : string matching and : +: : : : `print()` statements : +: : : : without suppressing : +: : : : the interactive : +: : : : pager. : +| **T6-06** | Symmetric | Medium | Mixing one output | +: : Execution : : format's dispatch : +: : Dispatching : : call with the inline : +: : : : procedural logic of : +: : : : the fallback format. : +| **T6-07** | CLI Argument | Medium | Overriding a | +: : Simulation over : : configuration : +: : Internal State : : parameter directly on : +: : Mutation : : the parsed namespace : +: : : : object. : +| **T6-08** | Deterministic | Medium | Iterating over a list | +: : Sorting of : : of accumulated : +: : Parallel : : multi-process results : +: : Execution : : without sorting. : +: : Results : : : +| **T6-09** | Strict Adherence | Medium | Adding a | +: : to Feature : : `--force-jobs` CLI : +: : Requests : : bypass flag : +: : : : simultaneously : +: : : : alongside the new : +: : : : `sync-j-max` manifest : +: : : : attribute. : +| **T6-10** | Minimal Logic in | High | Placing the entire | +: : Exception : : typo-correction : +: : Handlers : : logic, user prompts, : +: : : : and command reloading : +: : : : inside `except : +: : : : KeyError\:`. : +| **T6-11** | Unified Logging | Medium | Outputting warnings | +: : Protocol via : : directly to the : +: : Standard Logger : : `sys.stderr` stream : +: : : : using the built-in : +: : : : `print` function. : +| **T6-12** | Strict | High | Wrapping string-typed | +: : Translation of : : configuration : +: : Configuration : : variables inside : +: : Booleans : : `int()` and catching : +: : : : value exceptions to : +: : : : deduce defaults. : +| **T6-13** | Interruptibility | Critical | Placing raw time | +: : of Interactive : : suspension code : +: : Runtime Delays : : before a return : +: : : : statement without : +: : : : wrapping it in a trap : +: : : : for cancellation : +: : : : overrides. : +| **T6-14** | Strict | Critical | Consuming CLI option | +: : Pre-Access : : flags before the : +: : Initialization : : shared initialization : +: : of CLI Options : : pipeline populates : +: : : : default properties. : +| **T6-15** | Omission of | Medium | Redundantly | +: : Redundant : : configuring : +: : Optparse : : destination variables : +: : Destinations : : in CLI arguments. : +| **T6-16** | Subcommand | Medium | Writing | +: : Scoping in : : global-sounding : +: : Commit Messages : : commit messages for : +: : : : isolated subcommand : +: : : : patches. : +| **T6-17** | Module-Scoped | Medium | Redundantly prefixing | +: : Exception Naming : : the exception name : +: : : : with the module name : +: : : : inside the module : +: : : : definition. : +| **T6-18** | Explicit | High | Using terms like | +: : Terminology in : : 'worktree' in help : +: : Destructive CLI : : text without explicit : +: : Commands : : definition or : +: : : : examples. : +| **T6-19** | Fail-Fast | Medium | Checking for missing | +: : Argument : : positional arguments : +: : Validation in : : directly inside : +: : Subcommands : : `Execute()`. : +| **T6-20** | Respect Global | Medium | Unconditionally | +: : Verbosity Flags : : printing progress : +: : : : messages to stderr or : +: : : : stdout. : +| **T6-21** | Thread-Safe | Critical | Directly querying | +: : Terminal : : `sys.stderr.fileno()` : +: : Querying during : : inside long-running : +: : IO Capture : : concurrent loops. : +| **T6-22** | Global State via | Medium | Adding environment | +: : Persistent : : variable mappings : +: : Configuration : : into command-line : +: : over Environment : : parsers to support : +: : Variables : : 'headless' : +: : : : configurations. : +| **T6-23** | CLI Command | Medium | Adding project-wide | +: : Parity with Git : : or environmental : +: : Semantics : : metadata queries to : +: : : : commands designed for : +: : : : local working-tree : +: : : : analysis. : +| **T6-24** | Context-Aware | Medium | Triggering an | +: : Sync Operation : : immediate system exit : +: : Termination : : on an update failure. : +| **T6-25** | Explicit Exit | High | Using an empty | +: : Codes on CLI : : `return` to back out : +: : Cancellation : : of a declined CLI : +: : : : confirmation prompt. : +| **T6-26** | Non-Interactive | Medium | Hardcoding | +: : Safeties for : : interactive prompts : +: : Destructive : : without checking : +: : Commands : : configuration flags : +: : : : for automation modes. : +| **T6-27** | Strict | Medium | Accepting dependent | +: : Interdependent : : modifier options : +: : Argument : : unconditionally : +: : Validation : : without checking : +: : : : prerequisites. : +| **T6-28** | Actionable CLI | Medium | Failing a sync | +: : Error Messages : : operation with a : +: : for : : descriptive but : +: : Synchronization : : non-actionable error : +: : Failures : : message. : +| **T6-29** | Launcher Version | High | Modifying the | +: : Bumping for : : standalone launcher : +: : Important : : script with important : +: : Changes : : changes without : +: : : : incrementing its : +: : : : VERSION tuple. : + +-------------------------------------------------------------------------------- + +### Rules + +#### T6-01: Deterministic Machine-Readable Output Formats + +> **Rule:** Always expose strict, structured serialization options (e.g., JSON) +> for CLI tools used in automated pipelines to prevent reliance on brittle text +> parsing. +> +> **What:** CLI tools leveraged for inspection must expose strict, structured +> serialization (e.g., JSON) to negate reliance on unstable text parsing in +> CI/CD environments. +> +> **Applies To:** Command-line interface design, specifically subcommands +> utilized by build pipelines or external orchestrators. +> +> **Why:** Downstream automated pipelines were forced to retrieve configuration +> data by executing tools like `repo info` and using string matching (`grep`) +> against human-readable formatting, creating a brittle dependency susceptible +> to UI modifications. Failing to adhere to this typically results in **Pipeline +> Parsing Failure**. + +**Trap 1: Providing only aesthetically formatted terminal text for diagnostic +commands.** + +**Don't:** + +* Command executes and prints human-readable strings. Consumers must run `repo + info | grep "Manifest revision"` to extract variables. + +**Do:** + +* Integrate a `--format=json` command argument that bypasses human-readable + display logic and cleanly serializes underlying data models directly to + standard output. + +**Exceptions:** Interactive-only terminal prompts that are heavily stateful and +fundamentally incompatible with automation. + +-------------------------------------------------------------------------------- + +#### T6-02: Elimination of Niche Short Options + +> **Rule:** Never assign single-character short options for niche or rarely used +> flags to preserve namespace availability. +> +> **What:** Avoid assigning single-character short options for niche or rarely +> used command-line flags to prevent namespace bloat. +> +> **Applies To:** CLI argument parsing (e.g., `optparse` or `argparse` +> definitions). +> +> **Why:** Short options were being added arbitrarily for specific data +> summaries, leading to a cluttered short-option namespace and hindering future +> expansion. Failing to adhere to this typically results in **Cluttered CLI +> UX**. + +**Trap 1: Assigning a short option to a feature that isn't commonly used by +developers.** + +**Don't:** + +```python +p.add_option("-s", "--summary", action="store_true") +``` + +**Do:** + +```python +p.add_option("--include-summary", action="store_true") +``` + +-------------------------------------------------------------------------------- + +#### T6-03: Extensible Boolean Toggle Flags + +> **Rule:** Always implement paired positive/negative toggle flags for granular +> outputs rather than relying on rigid, single-purpose boolean switches. +> +> **What:** Conditional output sections must be controlled by paired +> positive/negative toggle flags (e.g., `--include-x` and `--no-include-x`) +> rather than single-purpose boolean flags. +> +> **Applies To:** CLI design and option definitions for output manipulation. +> +> **Why:** Rigid, single-purpose flags for rendering specific summaries limited +> extensibility and prevented developers from precisely tailoring outputs for +> scripts or automation. Failing to adhere to this typically results in **Rigid +> Output Rendering**. + +**Trap 1: Creating rigid, single-purpose boolean switches for granular output +components.** + +**Don't:** + +```python +p.add_option("--summary", action="store_true", help="show only manifest summary") +``` + +**Do:** + +```python +p.add_option("--include-summary", action="store_true", default=True) +p.add_option("--no-include-summary", dest="include_summary", action="store_false") +``` + +-------------------------------------------------------------------------------- + +#### T6-04: Context-Aware CLI Output Separators + +> **Rule:** Must conditionally verify that succeeding content blocks will render +> before printing visual formatting elements like horizontal separators. +> +> **What:** Structural visual elements, such as horizontal separators or +> newlines, must only be printed if the succeeding content block is actually +> verified to render. +> +> **Applies To:** CLI output rendering logic. +> +> **Why:** Output formatting logic printed hardcoded line separators after +> summaries, resulting in dangling visual artifacts when subsequent project +> details were dynamically excluded by user flags. Failing to adhere to this +> typically results in **Redundant Output Noise**. + +**Trap 1: Hardcoding visual formatting elements directly after a primary block +instead of conditionally checking the next block.** + +**Don't:** + +```python +print_summary() +print_separator() +if include_projects: + print_projects() +``` + +**Do:** + +```python +print_summary() +if not include_projects: + return +print_separator() +print_projects() +``` + +-------------------------------------------------------------------------------- + +#### T6-05: Output Format Standardization via Enums + +> **Rule:** Must manage and standardize target output formats using an explicit +> Python `Enum` class rather than brittle string matching. +> +> **What:** Command-line output formats (like TEXT or JSON) must be managed +> using an explicit Python Enum class and standardize formatting behaviors +> (e.g., disabling pagers for JSON, enforcing sort keys). +> +> **Applies To:** CLI data serialization and `OutputFormat` dispatching. +> +> **Why:** Unstandardized JSON serialization methods across subcommands resulted +> in inconsistent output spacing, failure to suppress interactive pagers, and +> brittle string-based format matching. Failing to adhere to this typically +> results in **Broken Machine Automation**. + +**Trap 1: Handling output formatting via raw string matching and `print()` +statements without suppressing the interactive pager.** + +**Don't:** + +```python +if opt.format == "json": + print(json.dumps(data)) +``` + +**Do:** + +```python +class OutputFormat(enum.Enum): + TEXT = enum.auto() + JSON = enum.auto() + +if output_format == OutputFormat.JSON: + sys.stdout.write(json.dumps(data, indent=2, sort_keys=True) + "\n") +``` + +-------------------------------------------------------------------------------- + +#### T6-06: Symmetric Execution Dispatching + +> **Rule:** Must symmetrically separate procedural logic into format-specific +> handlers rather than mixing inline logic with format conditional statements. +> +> **What:** Complex commands handling multiple output formats must keep the main +> execution function clean by symmetrically dispatching logic to format-specific +> handlers (e.g., `_ExecuteText` and `_ExecuteJson`). +> +> **Applies To:** Command entry points (e.g., the `Execute` method in subcommand +> classes). +> +> **Why:** Inline output rendering mixed with conditional format dispatching +> created monolithic execution methods that were difficult to maintain and test. +> Failing to adhere to this typically results in **Spaghetti Logic**. + +**Trap 1: Mixing one output format's dispatch call with the inline procedural +logic of the fallback format.** + +**Don't:** + +```python +def Execute(self, opt, args): + if opt.format == 'json': + return self._ExecuteJson(opt, args) + # 50 lines of inline text rendering logic... +``` + +**Do:** + +```python +def Execute(self, opt, args): + if opt.format == OutputFormat.JSON: + self._ExecuteJson(opt, args) + else: + self._ExecuteText(opt, args) +``` + +-------------------------------------------------------------------------------- + +#### T6-07: CLI Argument Simulation over Internal State Mutation + +> **Rule:** Always inject simulated execution parameters into the command-line +> `argv` arguments list rather than manually overwriting parsed attribute states +> during tests. +> +> **What:** When executing a command programmatically within a test, operational +> parameters (e.g., job counts) must be injected into the simulated `argv` +> string list rather than manually mutating the parsed options object. +> +> **Applies To:** Command testing and execution harnesses where parameters alter +> system behavior (e.g., multiprocessing threads). +> +> **Why:** Test cases were bypassing the CLI parser and manually setting +> `opts.jobs = 1` to force single-threaded execution. This prevented the test +> from exercising the command's full option-parsing and validation lifecycle. +> Failing to adhere to this typically results in **Test Invalidation / False +> Positives**. + +**Trap 1: Overriding a configuration parameter directly on the parsed namespace +object.** + +**Don't:** + +```python +opts, args = cmd.OptionParser.parse_args(argv) +cmd.CommonValidateOptions(opts, args) +opts.jobs = 1 +cmd.Execute(opts, args) +``` + +**Do:** + +```python +opts, args = cmd.OptionParser.parse_args(argv + ["--jobs=1"]) +cmd.CommonValidateOptions(opts, args) +cmd.Execute(opts, args) +``` + +-------------------------------------------------------------------------------- + +#### T6-08: Deterministic Sorting of Parallel Execution Results + +> **Rule:** Must explicitly sort accumulated payload data from asynchronous +> thread loops before broadcasting standard outputs or logs. +> +> **What:** Elements collected asynchronously from a multiprocessing pool must +> be explicitly sorted before being emitted to standard output or logging +> pipelines. +> +> **Applies To:** Command-line output rendering and any loop iterating over +> aggregated parallel results. +> +> **Why:** Warning messages regarding bloated project directories were printed +> based on the order that asynchronous threads returned their payload, leading +> to flaky UX and non-deterministic command outputs. Failing to adhere to this +> typically results in **Non-Deterministic Console Output**. + +**Trap 1: Iterating over a list of accumulated multi-process results without +sorting.** + +**Don't:** + +```python +for project_name in self._bloated_projects: + logger.warning(f'warning: Project {project_name} is bloated.') +``` + +**Do:** + +```python +for project_name in sorted(self._bloated_projects): + logger.warning(f'warning: Project {project_name} is bloated.') +``` + +-------------------------------------------------------------------------------- + +#### T6-09: Strict Adherence to Feature Requests + +> **Rule:** Never implement speculative bypass flags, preemptive CLI overrides, +> or configurations without a documented and verifiable user request. +> +> **What:** Avoid preemptively implementing fallback features, command-line +> overrides, or configurations without a documented user request. +> +> **Applies To:** CLI argument parsers and manifest parsing engines. +> +> **Why:** Speculative feature additions, like bypass flags for manifest limits, +> caused feature creep and bloated the CLI toolset with unused, undocumented +> edge cases. Failing to adhere to this typically results in **Feature Creep**. + +**Trap 1: Adding a `--force-jobs` CLI bypass flag simultaneously alongside the +new `sync-j-max` manifest attribute.** + +**Don't:** + +* Implementing an override flag in `main.py` just in case users want to bypass + the newly added manifest restriction. + +**Do:** + +* Deploying the manifest constraint directly without a CLI override, and + documenting it in `docs/manifest-format.md`. Wait for a bug tracker request + for an override flag. + +-------------------------------------------------------------------------------- + +#### T6-10: Minimal Logic in Exception Handlers + +> **Rule:** Must restrict `try...except` blocks entirely to targeted error +> handling and never bury structural flow control or terminal loops within them. +> +> **What:** Exception handling blocks (`try...except`) must only contain the +> specific error-handling logic and never encapsulate complex business logic, +> terminal loops, or major execution routing. +> +> **Applies To:** Command dispatcher routing and system control flows. +> +> **Why:** Burying significant CLI routing tasks—like spelling suggestions, +> autocorrection countdowns, and subprocess recursion—inside a `KeyError` +> handler obscured program logic and suppressed nested tracebacks. Failing to +> adhere to this typically results in **Opaque Routing State**. + +**Trap 1: Placing the entire typo-correction logic, user prompts, and command +reloading inside `except KeyError:`.** + +**Don't:** + +```python +try: + cmd = self.commands[name]() +except KeyError: + # 50 lines of complex autocorrection logic + return self._RunLong(...) +``` + +**Do:** + +```python +if name not in self.commands: + name = self._autocorrect_command_name(name) + if not name: + return 1 +# Execute minimal command logic outside exception +cmd = self.commands[name]() +``` + +-------------------------------------------------------------------------------- + +#### T6-11: Unified Logging Protocol via Standard Logger + +> **Rule:** Dispatch all console messages using the standard centralized logging +> framework. Never rely on raw `print` statements mapped directly to terminal +> error streams. +> +> **What:** All console outputs must be dispatched via the system's centralized +> `RepoLogger` mechanism, completely replacing raw terminal standard-error +> prints. +> +> **Applies To:** CLI presentation logic and application-wide console +> communications. +> +> **Why:** The use of raw `print(file=sys.stderr)` bypassed the system's unified +> logging configuration, resulting in unformatted outputs and broken terminal +> interactivity. Failing to adhere to this typically results in **Unmanaged +> Output Spillage**. + +**Trap 1: Outputting warnings directly to the `sys.stderr` stream using the +built-in `print` function.** + +**Don't:** + +```python +print(f"WARNING: You called a command named '{name}'", file=sys.stderr) +``` + +**Do:** + +```python +logger.warning("You called a repo command named '%s'", name) +``` + +**Exceptions:** Prompts that require strict, synchronously unformatted +interaction via `input()`. + +-------------------------------------------------------------------------------- + +#### T6-12: Strict Translation of Configuration Booleans + +> **Rule:** Always cast string-based Git configurations to lowercase and map +> them strictly against Git's recognized array of boolean equivalents. +> +> **What:** Parsers querying Git configuration properties must implement strict +> hardcoding to safely capture Git's expansive list of non-standard boolean +> string representations (e.g., 'yes', 'on', 'true', 'off'). +> +> **Applies To:** Configuration parsers mapping user `git config` state to +> internal state variables. +> +> **Why:** Using standard integer casts on retrieved git configuration strings +> caused runtime failures when users specified string-based boolean values +> sanctioned by the official Git system. Failing to adhere to this typically +> results in **Configuration Casting Panic**. + +**Trap 1: Wrapping string-typed configuration variables inside `int()` and +catching value exceptions to deduce defaults.** + +**Don't:** + +```python +try: + autocorrect = int(autocorrect) +except ValueError: + autocorrect = 0 +``` + +**Do:** + +```python +autocorrect = str(autocorrect).lower() +if autocorrect in ("0", "false", "off", "no", "show"): + autocorrect = 0 +elif autocorrect in ("1", "true", "on", "yes", "immediate"): + autocorrect = -1 +``` + +-------------------------------------------------------------------------------- + +#### T6-13: Interruptibility of Interactive Runtime Delays + +> **Rule:** Must wrap artificially injected execution delays in handlers that +> correctly detect and gracefully back out of operations upon a keyboard +> interrupt signal. +> +> **What:** Any thread-blocking delay generated by the system (e.g., +> autocorrection warning timers) must actively handle thread interrupt signals +> so users can gracefully cancel impending state mutations. +> +> **Applies To:** Thread blocks, specifically interactive or automated execution +> delays. +> +> **Why:** Forced sequential time sleeps preceding an automated command +> execution trapped users. If a user detected an erroneous correction during the +> sleep interval, initiating a keyboard interrupt would fail to abort execution, +> resulting in destructive command routing. Failing to adhere to this typically +> results in **Irrevocable Execution**. + +**Trap 1: Placing raw time suspension code before a return statement without +wrapping it in a trap for cancellation overrides.** + +**Don't:** + +```python +time.sleep(delay) +return assumed_command +``` + +**Do:** + +```python +try: + time.sleep(delay) +except KeyboardInterrupt: + return None +return assumed_command +``` + +-------------------------------------------------------------------------------- + +#### T6-14: Strict Pre-Access Initialization of CLI Options + +> **Rule:** Never retrieve attributes from a parsed options namespace until the +> shared subcommand option validation layer has fully completed processing. +> +> **What:** Command-line option attributes (e.g., `copts.verbose`) must strictly +> be accessed only after the options object has fully passed through the common +> subcommand validation layer. +> +> **Applies To:** CLI entry points (`main.py`) and option routing blocks. +> +> **Why:** Accessing attributes on the configuration options object prior to +> invoking `CommonValidateOptions` (which populates global defaults) resulted in +> `AttributeError` crashes, completely breaking primary application functions +> like `repo sync`. Failing to adhere to this typically results in +> **AttributeError / Process Crash**. + +**Trap 1: Consuming CLI option flags before the shared initialization pipeline +populates default properties.** + +**Don't:** + +```python +git_trace2_event_log.verbose = copts.verbose +cmd.CommonValidateOptions(copts, cargs) +cmd.ValidateOptions(copts, cargs) +``` + +**Do:** + +```python +cmd.CommonValidateOptions(copts, cargs) +cmd.ValidateOptions(copts, cargs) +git_trace2_event_log.verbose = copts.verbose +``` + +-------------------------------------------------------------------------------- + +#### T6-15: Omission of Redundant Optparse Destinations + +> **Rule:** Avoid manually injecting `dest=` bindings in argument declarations +> if the implicit destination perfectly mirrors the target variable name. +> +> **What:** Do not explicitly pass `dest=` arguments in CLI option definitions +> if the value identically matches `optparse`'s implicit fallback behavior +> (which strips leading dashes and replaces inner dashes with underscores). +> +> **Applies To:** Command-line argument definitions using the `optparse` library +> within `subcmds/`. +> +> **Why:** Developers repeatedly specified `dest=` overrides that exactly +> mirrored the auto-generated variable name, adding unnecessary boilerplate and +> triggering automated regression test failures. Failing to adhere to this +> typically results in **Code Bloat / Test Failure**. + +**Trap 1: Redundantly configuring destination variables in CLI arguments.** + +**Don't:** + +```python +p.add_option( + "--no-interleaved", + dest="no_interleaved", + action="store_false", +) +``` + +**Do:** + +```python +p.add_option( + "--no-interleaved", + action="store_false", +) +``` + +-------------------------------------------------------------------------------- + +#### T6-16: Subcommand Scoping in Commit Messages + +> **Rule:** Always prefix your commit messages with the explicit CLI subcommand +> or modular subsystem the patch directly targets. +> +> **What:** Git commit message subject lines must explicitly scope changes to +> the CLI subcommand or module they affect (e.g., `sync: ` or `project: `). +> +> **Applies To:** Commit message authoring. +> +> **Why:** Vague commit messages such as 'Default to interleaved mode' +> obfuscated history. Scoping was mandated to ensure maintainers could quickly +> trace changes within the mono-repo. Failing to adhere to this typically +> results in **Unclear Git History**. + +**Trap 1: Writing global-sounding commit messages for isolated subcommand +patches.** + +**Don't:** + +* Default to interleaved mode + +**Do:** + +* sync: Default to interleaved mode + +**Exceptions:** Changes that structurally alter the core framework or affect +multiple commands concurrently. + +-------------------------------------------------------------------------------- + +#### T6-17: Module-Scoped Exception Naming + +> **Rule:** Declare isolated module exceptions natively as `Error` instead of +> prefixing them with the resident module's name. +> +> **What:** Custom exceptions specific to a module (especially subcommands) +> should be named `Error` rather than prefixing them with the module name. +> +> **Applies To:** Module-level exception definitions, specifically within +> subcommand implementations. +> +> **Why:** Developers inconsistently named module exceptions (e.g., `WipeError` +> in `wipe.py`), violating the style guide and polluting the namespace upon +> import. Failing to adhere to this typically results in **Style Guide Violation +> / API Inconsistency**. + +**Trap 1: Redundantly prefixing the exception name with the module name inside +the module definition.** + +**Don't:** + +```python +# inside wipe.py +class WipeError(RepoExitError): + pass +``` + +**Do:** + +```python +# inside wipe.py +class Error(RepoExitError): + pass +``` + +-------------------------------------------------------------------------------- + +#### T6-18: Explicit Terminology in Destructive CLI Commands + +> **Rule:** Must rigorously define nuanced Git terminology natively within the +> terminal help output if a command has destructive capabilities. +> +> **What:** Destructive commands must clearly define ambiguous terms (like +> 'worktree') in their help documentation and provide concrete usage examples. +> +> **Applies To:** CLI help text, specifically `helpDescription` properties in +> `Command` subclasses executing file deletion. +> +> **Why:** Users misunderstood destructive operations because terminology +> overlapped with internal Git jargon, increasing the risk of accidental +> repository deletions. Failing to adhere to this typically results in +> **Accidental Data Loss**. + +**Trap 1: Using terms like 'worktree' in help text without explicit definition +or examples.** + +**Don't:** + +* helpDescription = "The repo wipe command removes the specified projects from + the worktree." + +**Do:** + +* helpDescription = "The repo wipe command removes the specified projects from + the worktree (the checked out source code)... \n\nExamples:\n %prog + <project>..." + +-------------------------------------------------------------------------------- + +#### T6-19: Fail-Fast Argument Validation in Subcommands + +> **Rule:** Must perform strict threshold checks for required argument counts at +> the validation layer before the command is permitted to execute. +> +> **What:** Validation checks for mandatory command-line arguments must occur in +> the `ValidateOptions` hook rather than within the core `Execute` block. +> +> **Applies To:** All `Command` subclasses. +> +> **Why:** Performing validation in the execution body delayed error feedback, +> triggering unnecessary setup logic and violating the command lifecycle +> architecture. Failing to adhere to this typically results in **Delayed +> Execution Errors / Architectural Inconsistency**. + +**Trap 1: Checking for missing positional arguments directly inside +`Execute()`.** + +**Don't:** + +```python +def Execute(self, opt, args): + if not args: + raise UsageError("no projects specified") + # ... execution logic +``` + +**Do:** + +```python +def ValidateOptions(self, opt, args): + if not args: + self.OptionParser.error("no projects specified") + +def Execute(self, opt, args): + # ... execution logic safely assumes valid args +``` + +-------------------------------------------------------------------------------- + +#### T6-20: Respect Global Verbosity Flags + +> **Rule:** Always wrap non-critical progress prints or logging updates behind +> global verbosity checks. +> +> **What:** Commands must remain silent by default. Output must be guarded by +> checking the global `opt.verbose` flag, preventing log noise during standard +> execution. +> +> **Applies To:** All `Command` subclasses executing file operations or progress +> updates. +> +> **Why:** Commands were unconditionally printing internal states (e.g., +> directory deletions), cluttering terminal output for users expecting standard +> silent behavior. Failing to adhere to this typically results in **Terminal +> Spam / UX Degradation**. + +**Trap 1: Unconditionally printing progress messages to stderr or stdout.** + +**Don't:** + +```python +print(f"Deleting objects directory: {objdir}", file=sys.stderr) +``` + +**Do:** + +```python +if opt.verbose: + print(f"Deleting objects directory: {objdir}", file=sys.stderr) +``` + +-------------------------------------------------------------------------------- + +#### T6-21: Thread-Safe Terminal Querying during IO Capture + +> **Rule:** Must query terminal geometry using a safely cached baseline file +> descriptor to prevent crash failures when the standard error stream is +> redirected to memory buffers. +> +> **What:** Background threads that query terminal properties (like width) must +> reference a persistently cached file descriptor of the original `sys.stderr`, +> as the global stream can be temporarily redirected to in-memory objects +> lacking file descriptors. +> +> **Applies To:** progress.py, UI monitoring loops, and subprocesses executed +> during `stderr`/`stdout` redirection. +> +> **Why:** When running parallel processes that captured output into an +> `io.StringIO()` buffer, a background progress thread attempting to dynamically +> measure the terminal width via `sys.stderr.fileno()` triggered an +> `io.UnsupportedOperation: fileno` exception, crashing the entire UI loop. +> Failing to adhere to this typically results in **Thread Crash / +> io.UnsupportedOperation**. + +**Trap 1: Directly querying `sys.stderr.fileno()` inside long-running concurrent +loops.** + +**Don't:** + +```python +# BAD: Fails if stderr is mocked to memory +col = os.get_terminal_size(sys.stderr.fileno()).columns +``` + +**Do:** + +```python +# GOOD: Querying a module-level cached reference mapped at boot +col = os.get_terminal_size(_STDERR.fileno()).columns +``` + +-------------------------------------------------------------------------------- + +#### T6-22: Global State via Persistent Configuration over Environment Variables + +> **Rule:** Never propagate novel OS-level environment variables to modify +> executable behavior, strictly enforcing the existing `.repoconfig/config` +> framework instead. +> +> **What:** Global user or CI settings (e.g., Git-LFS defaults) must be managed +> using the explicit `.repoconfig/config` framework. Do not introduce novel +> OS-level environment variables to modify executable behavior. +> +> **Applies To:** Configuration parsers, command-line argument mapping, and +> option handlers across the CLI. +> +> **Why:** A contributor attempted to add a `REPO_GIT_LFS` environment variable +> to ease CI automation. Maintainers rejected the patch, citing that the +> proliferation of disjointed environment variables builds technical debt +> compared to maintaining a single, consistent file-based configuration layer. +> Failing to adhere to this typically results in **Configuration Fragmentation / +> Tech Debt**. + +**Trap 1: Adding environment variable mappings into command-line parsers to +support 'headless' configurations.** + +**Don't:** + +* Read configuration defaults from `os.environ.get("REPO_GIT_LFS")` alongside + CLI flags. + +**Do:** + +* Instruct users to provision a `~/.repoconfig/config` file in automated + setups instead of changing process environment logic. + +**Exceptions:** Legacy environment variables (e.g., `REPO_URL`) remain supported +for backward compatibility but are explicitly designated as technical debt. + +-------------------------------------------------------------------------------- + +#### T6-23: CLI Command Parity with Git Semantics + +> **Rule:** Tightly bind local CLI utility semantics and argument definitions to +> match analogous Git operations explicitly. +> +> **What:** Internal CLI command responsibilities and user-facing terminology +> must align with established Git semantics and project-wide conventions. +> +> **Applies To:** Command-line interface modules (e.g., `subcmds/status.py`, +> `subcmds/info.py`). +> +> **Why:** Global metadata (like superproject commit IDs) was mistakenly added +> to `repo status`, which cluttered the local working-tree status. Additionally, +> the term 'hash' was used instead of the standard 'revision'. Failing to adhere +> to this typically results in **UX Inconsistency / Clutter**. + +**Trap 1: Adding project-wide or environmental metadata queries to commands +designed for local working-tree analysis.** + +**Don't:** + +* Injecting global repository state (e.g., `--superproject-hash`) into the + `status` command, which breaks parity with `git status`. + +**Do:** + +* Relocating environmental and global metadata display to the `info` command, + integrating it cleanly into the default summary header. + +**Trap 2: Using non-standard terminology for Git object identifiers in +user-facing CLI flags.** + +**Don't:** + +```python +# BAD: Using 'hash' which is inconsistent with existing repo terminology +p.add_option("--superproject-hash", help="print the superproject hash") +``` + +**Do:** + +```python +# GOOD: Using 'revision' +p.add_option("--superproject-rev", help="print the superproject revision") +``` + +-------------------------------------------------------------------------------- + +#### T6-24: Context-Aware Sync Operation Termination + +> **Rule:** Must catch and defer local sync iteration failures gracefully rather +> than forcing immediate unrecoverable exits across all active streams. +> +> **What:** Non-critical update failures during local sync iterations must set +> an error flag but must only force a hard exit if the user explicitly provided +> the `--fail-fast` CLI argument. +> +> **Applies To:** All local project iteration workflows within +> `subcmds/sync.py`. +> +> **Why:** A single project failing an isolated sub-task (like a symlink update) +> would immediately trigger `sys.exit(1)`, aborting the entire pipeline for all +> other projects and ignoring standard error aggregation strategies. Failing to +> adhere to this typically results in **Premature Pipeline Termination**. + +**Trap 1: Triggering an immediate system exit on an update failure.** + +**Don't:** + +```python +if self.UpdateCopyLinkfileList(): + print('\nerror: Local update copyfile or linkfile failed.', file=sys.stderr) + sys.exit(1) +``` + +**Do:** + +```python +if not self.UpdateCopyLinkfileList(): + err_event.set() + err_update_linkfiles = True + if opt.fail_fast: + print('\nerror: Local update copyfile or linkfile failed.', file=sys.stderr) + sys.exit(1) +``` + +-------------------------------------------------------------------------------- + +#### T6-25: Explicit Exit Codes on CLI Cancellation + +> **Rule:** Always force the process to yield a non-zero exit code if an +> interactive confirmation prompt is declined or cancelled. +> +> **What:** Interactive CLI workflows that are cancelled or declined by the user +> must immediately return a non-zero exit code to signal termination to parent +> processes. +> +> **Applies To:** All interactive subcommands (`gc`, `sync` with prompts, etc.). +> +> **Why:** If a tool cleanly exited (returning None / 0) after a user denied a +> destructive action prompt, shell automation wrappers and CI systems falsely +> assumed the destructive cleanup successfully completed. Failing to adhere to +> this typically results in **Silent Automation Failure**. + +**Trap 1: Using an empty `return` to back out of a declined CLI confirmation +prompt.** + +**Don't:** + +```python +ask = input("Proceed? [y/N] ") +if ask.lower() != "y": + return +``` + +**Do:** + +```python +ask = input("Proceed? [y/N] ") +if ask.lower() != "y": + return 1 +``` + +-------------------------------------------------------------------------------- + +#### T6-26: Non-Interactive Safeties for Destructive Commands + +> **Rule:** Must implement explicit automation bypass flags for any command +> capable of irreversibly destroying system state. +> +> **What:** Any command capable of destroying state (e.g., garbage collection, +> wiping local commits) must provide both a `--dry-run` and a `--yes` (or +> `--force`) flag to support safe CI automation. +> +> **Applies To:** Command-line definitions for potentially destructive +> operations. +> +> **Why:** Tools that rely exclusively on `input()` prompts block automated +> scripts indefinitely. By explicitly enforcing non-interactive modes, scripts +> can safely trigger automated cleanups or log dry-runs without freezing. +> Failing to adhere to this typically results in **Automation Hang / CI Pipeline +> Freeze**. + +**Trap 1: Hardcoding interactive prompts without checking configuration flags +for automation modes.** + +**Don't:** + +* Prompting the user with `input('Proceed?')` unconditionally before executing + a destructive delete. + +**Do:** + +* Checking `if opt.dry_run:` to skip execution, or `if opt.yes:` to skip the + confirmation prompt. + +-------------------------------------------------------------------------------- + +#### T6-27: Strict Interdependent Argument Validation + +> **Rule:** Always enforce strict prerequisites checks when parsing command-line +> flags intended to modify or inherit context from other flags. +> +> **What:** Command-line options that exist solely to modify or qualify another +> specific command-line option must be explicitly validated to raise an error if +> their prerequisite option is absent. +> +> **Applies To:** CLI argument parsing routines (`subcmds/init.py`, +> `ValidateOptions`). +> +> **Why:** A new `--manifest-upstream-branch` argument was added to qualify a +> provided commit hash. However, without strict validation, users could supply +> it independently, leading to ambiguous or unhandled configuration states. +> Failing to adhere to this typically results in **CLI Ambiguity / Unhandled +> Option State**. + +**Trap 1: Accepting dependent modifier options unconditionally without checking +prerequisites.** + +**Don't:** + +* Accepting `--manifest-upstream-branch` in the CLI parser and directly + assigning it to the internal state regardless of the presence of + `--manifest-branch`. + +**Do:** + +* Explicitly checking the parsed options and raising an `OptionParser.error` + if the modifier is present but the target `--manifest-branch` is null. + +-------------------------------------------------------------------------------- + +#### T6-28: Actionable CLI Error Messages for Synchronization Failures + +> **Rule:** Embed direct, explicit commands and flag references within terminal +> error payloads to rescue users from failing execution states. +> +> **What:** When a terminal failure occurs during CLI operations, the error +> payload must provide specific, actionable flags or manual steps for the user +> to resolve the failing state. +> +> **Applies To:** Sync command error reporting (`subcmds/sync.py`, +> `project.py`). +> +> **Why:** When a local branch had published commits but was behind upstream, +> the sync failed with a generic 'punt' message, leaving users unaware that +> passing `--rebase` could force the sync. Failing to adhere to this typically +> results in **User Friction / Dead End**. + +**Trap 1: Failing a sync operation with a descriptive but non-actionable error +message.** + +**Don't:** + +```python +fail(LocalSyncFail( + "branch %s is published (but not merged) and is now %d commits behind" + % (branch.name, len(upstream_gain)), + project=self.name, +)) +``` + +**Do:** + +```python +fail(LocalSyncFail( + "branch %s is published (but not merged) and is now %d commits behind. " + "Fix this manually or rerun with the --rebase option to force a rebase." + % (branch.name, len(upstream_gain)), + project=self.name, +)) +``` + +-------------------------------------------------------------------------------- + +#### T6-29: Launcher Script Version Upgrades for Important Changes + +> **Rule:** Increment the `VERSION` tuple within the standalone `repo` launcher script when making important or significant functional changes. +> +> **What:** If you modify the standalone `repo` launcher file with an important or large change (e.g., updates to Python version checks, significant logic fixes, or major argument parsing updates), you must increment the `VERSION = (X, Y)` tuple. +> +> **Applies To:** Standalone `repo` launcher script, release engineering workflows. +> +> **Why:** The repo source code entry point (`main.py`) compares the runner wrapper version to the copy of the launcher in the source tree to prompt users to upgrade. If an important launcher modification is submitted without a corresponding version bump, users executing older wrappers won't receive the required update prompts. Failing to adhere to this typically results in **Launcher Version Skew / Missing Update Warnings**. +> +> **Exceptions:** Minor or cosmetic changes (like fixing typos in comments or minor docstrings) do not require a version bump. + +**Trap 1: Submitting a major or important patch to the `repo` launcher script without updating its `VERSION` definition.** + +**Don't:** + +```python +# Making significant changes to Python version handling logic +# but leaving VERSION unchanged: +VERSION = (2, 65) +``` + +**Do:** + +```python +# Incrementing the minor version tuple to signal the update: +VERSION = (2, 66) +``` + +-------------------------------------------------------------------------------- + +### Cross-Domain Dependencies + +* **Upstream:** T1 | Concurrent Synchronization & IPC - *CLI rendering relies + on sorted, deterministic results emitted from asynchronous multiprocessing + pools.* +* **Downstream:** T3 | Subprocess Git Integration & Error Translation - + *Command-line terminology and state configurations must tightly align with + wrapped Git subprocess semantics.* + +## Chapter: Repo Hooks Framework + +**Context:** The Repo Hooks Framework governs the execution, parameter +validation, and lifecycle management of user-defined scripts within the +repository ecosystem. It ensures seamless integration of extensions like +`post-sync` or `pre-upload` while strictly isolating their execution failures +from core operational workflows. + +### Summary + +| Rule ID | Principle / | Priority | Primary Symptom / Trap | +: : Constraint : : : +| :-------- | :----------------- | :------- | :------------------------------- | +| **T7-01** | Explicit | Critical | Passing a new variable to | +: : Registration of : : `hook.Run()` without : +: : Hook Parameters in : : whitelisting it in the : +: : API Contract : : `_API_ARGS` map. : +| **T7-02** | Floating-Point | High | Casting float calculations | +: : Precision for Hook : : directly to integers before : +: : Telemetry Metrics : : passing to downstream systems. : +| **T7-03** | Non-Blocking | High | Using raw `subprocess` calls to | +: : Post-Sync Hook : : run custom scripts and tying the : +: : Integration : : command's exit code to the main : +: : : : tool's exit code. : +| **T7-04** | Dynamic CLI Option | Medium | Bypassing standard option | +: : Injection for Repo : : parsers by passing a mock object : +: : Hooks : : with hardcoded values into hook : +: : : : factory methods. : +| **T7-05** | Boolean Evaluation | High | Wrapping a framework execution | +: : for Repo Hook : : method in a generic exception : +: : Execution Status : : handler. : +| **T7-06** | Universal Trigger | High | Documenting hook triggers based | +: : Specification for : : on an idealized use-case (a full : +: : Execution : : checkout) rather than the : +: : Lifecycle : : literal programmatic trigger. : +| **T7-07** | Forward-Compatible | High | Defining a hook entrypoint with | +: : kwargs for Repo : : a strict argument list that : +: : Hooks Signatures : : cannot gracefully handle new : +: : : : inputs. : + +-------------------------------------------------------------------------------- + +### Rules + +#### T7-01: Explicit Registration of Hook Parameters in API Contract + +> **Rule:** Always register new metadata or arguments intended for user-defined +> lifecycle scripts within the internal `_API_ARGS` validation dictionary. +> +> **What:** Any new metadata or argument passed into user-defined lifecycle +> scripts must be explicitly defined in the hook's internal validation +> dictionary (`_API_ARGS`). +> +> **Applies To:** Repo Hooks Framework (`hooks.py` and Hook Runners). +> +> **Why:** New arguments sent to the post-sync execution context triggered +> internal exceptions because the core application strictly validates `**kwargs` +> against a registered list of supported arguments. Failing to adhere to this +> typically results in **Runtime Hook Failure**. + +**Trap 1: Passing a new variable to `hook.Run()` without whitelisting it in the +`_API_ARGS` map.** + +**Don't:** + +```python +# In subcmds/sync.py +hook.Run(repo_topdir=self.client.topdir, sync_duration_seconds=dur) + +# In hooks.py (Missing registration) +_API_ARGS = { + 'post-sync': {'repo_topdir'} +} +``` + +**Do:** + +```python +# In hooks.py +_API_ARGS = { + 'post-sync': {'repo_topdir', 'sync_duration_seconds'} +} +``` + +#### T7-02: Floating-Point Precision for Hook Telemetry Metrics + +> **Rule:** Must pass system telemetry and operational duration metrics to +> downstream extension hooks as precise floating-point values. +> +> **What:** System telemetry, specifically operational duration metrics, must be +> passed to downstream extension hooks as granular floating-point values. The +> executing system must not truncate or cast telemetry to integers. +> +> **Applies To:** Performance telemetry generation and hook contexts. +> +> **Why:** Internal performance timers originally cast their delta outputs to +> integers before emitting the context to the hook system. This caused a loss of +> sub-second precision, limiting telemetry usefulness. Failing to adhere to this +> typically results in **Telemetry Data Loss**. + +**Trap 1: Casting float calculations directly to integers before passing to +downstream systems.** + +**Don't:** + +```python +sync_duration_seconds = time.time() - start_time +self._RunPostSyncHook(opt, sync_duration_seconds=int(sync_duration_seconds)) +``` + +**Do:** + +```python +sync_duration_seconds = time.time() - start_time +self._RunPostSyncHook(opt, sync_duration_seconds=sync_duration_seconds) +``` + +#### T7-03: Non-Blocking Post-Sync Hook Integration + +> **Rule:** Never allow post-operation custom hooks to block or alter the +> success state of the core operation. +> +> **What:** Custom hooks executed strictly after an operation finishes (like +> `post-sync`) must run via the encapsulated `RepoHook` framework and be +> entirely non-blocking, so their failure does not negatively affect the core +> operation's recorded success state. +> +> **Applies To:** subcmds/sync.py and `RepoHook` integrations executing after +> repository data modifications. +> +> **Why:** Teams needed a reliable way to provision local Git `commit-msg` hooks +> even if developers bypassed `repo upload` with manual `git push`. A +> `post-sync` hook was added, but the rigid requirement was that if the user +> script failed, the actual project synchronization must still report as a +> success to avoid corrupting UX. Failing to adhere to this typically results in +> **Blocked Sync Operations**. + +**Trap 1: Using raw `subprocess` calls to run custom scripts and tying the +command's exit code to the main tool's exit code.** + +**Don't:** + +```python +# BAD: Post-action failure corrupts the sync outcome +if subprocess.run(["run_hooks.sh"]).returncode != 0: + raise SyncError("Hook failed") +``` + +**Do:** + +```python +# GOOD: Managed execution safely wrapping and suppressing exceptions +hook = RepoHook("post-sync") +hook.Run() # Designed strictly to warn on failure +``` + +#### T7-04: Dynamic CLI Option Injection for Repo Hooks + +> **Rule:** Always utilize the RepoHook option group injection framework for +> command-line integration rather than synthesizing dummy configuration objects. +> +> **What:** Command-line integration for new hooks must utilize the RepoHook +> option group injection framework rather than synthesizing dummy configuration +> objects. +> +> **Applies To:** Repo Hooks Framework; specifically subcommand definitions +> involving hook invocations (e.g., `subcmds/sync.py`). +> +> **Why:** When integrating the post-sync hook, a developer initially created a +> minimal 'DummyOpt' object to satisfy parameter checks because the sync command +> lacked native hook flags. This diverged from standard hook architecture and +> could cause attribute errors. Failing to adhere to this typically results in +> **AttributeError / Option Divergence**. + +**Trap 1: Bypassing standard option parsers by passing a mock object with +hardcoded values into hook factory methods.** + +**Don't:** + +```python +# BAD: Using a dummy object to satisfy missing CLI arguments +class DummyOpt: + bypass_hooks = False + allow_all_hooks = True + ignore_hooks = True + +hook = RepoHook.FromSubcmd(opt=DummyOpt(), ...) +``` + +**Do:** + +```python +# GOOD: Injecting the standard hook option group into the parser +RepoHook.AddOptionGroup(parser, 'post-sync') + +# Later during execution, pass the actual resolved options: +hook = RepoHook.FromSubcmd(opt=actual_opt, ...) +``` + +#### T7-05: Boolean Evaluation for Repo Hook Execution Status + +> **Rule:** Always evaluate hook execution success using the boolean return +> value of `Run()`, never by wrapping the call in external exception handlers. +> +> **What:** Execution wrappers for Repo Hooks must rely on the boolean return +> value of `Run()` to evaluate success, rather than wrapping the call in broad +> `try/except Exception` blocks. +> +> **Applies To:** Repo Hooks Framework; specifically the invocation layer for +> any hook type (`post-sync`, `pre-upload`). +> +> **Why:** A developer wrapped the hook execution in a broad Exception catch +> block. The hook framework inherently traps and standardizes exceptions +> internally; wrapping it externally duplicates logic, masks context, and leads +> to silent failure processing. Failing to adhere to this typically results in +> **Swallowed Exceptions / Logic Duplication**. + +**Trap 1: Wrapping a framework execution method in a generic exception +handler.** + +**Don't:** + +```python +# BAD: Redundant outer try/except block swallowing state +try: + hook.Run() +except Exception as e: + print(f"Warning: hook failed: {e}") +``` + +**Do:** + +```python +# GOOD: Relying on the established boolean return contract +success = hook.Run() +if not success: + print("Warning: hook reported failure.") +``` + +#### T7-06: Universal Trigger Specification for Execution Lifecycle + +> **Rule:** Must document the exact, literal programmatic triggers for hooks +> rather than relying on idealized use-cases like full checkouts. +> +> **What:** Hook documentation must strictly delineate the exact success states +> that trigger execution, explicitly noting conditions where operational state +> (like a local checkout) might be bypassed or skipped. +> +> **Applies To:** Repo Hooks API contracts and documentation +> (`docs/repo-hooks.md`). +> +> **Why:** Users of the new post-sync hook might assume it only runs after a +> complete workspace checkout. However, it effectively fires on any +> zero-exit-code sync operation, including network-only fetches where the local +> filesystem state remains completely untouched. Failing to adhere to this +> typically results in **State Assumption Bugs**. + +**Trap 1: Documenting hook triggers based on an idealized use-case (a full +checkout) rather than the literal programmatic trigger.** + +**Don't:** + +* Documenting that the hook is to install post-processing tasks specifically + for successful full checkouts. + +**Do:** + +* Documenting that the hook runs when `repo sync` completes without errors, + explicitly warning that this does not guarantee all projects were synced or + checked out (e.g., `repo sync -n` performs network fetches only). + +#### T7-07: Forward-Compatible kwargs for Repo Hooks Signatures + +> **Rule:** Always design hook script entrypoints to accept variable keyword +> arguments (`**kwargs`) to ensure forward compatibility with future framework +> expansions. +> +> **What:** All repository hook script entrypoints (`main` functions) must +> accept variable keyword arguments (`**kwargs`) to maintain forward +> compatibility with future API augmentations. +> +> **Applies To:** Repo Hooks scripts and internal examples (e.g., +> `post-sync.py`, `pre-upload.py`). +> +> **Why:** Historically, if the `repo` tool evolved to pass new environmental +> context (like `repo_topdir`), user-defined scripts with strict function +> signatures would crash with a TypeError. Failing to adhere to this typically +> results in **TypeError / API Breakage**. + +**Trap 1: Defining a hook entrypoint with a strict argument list that cannot +gracefully handle new inputs.** + +**Don't:** + +```python +# BAD: Strict signature that will crash on API update +def main(repo_topdir=None): + print("Running tasks...") +``` + +**Do:** + +```python +# GOOD: Accepts forward-compatible **kwargs +def main(repo_topdir=None, **kwargs): + """ + Args: + kwargs: Leave this here for forward-compatibility. + """ + print("Running tasks...") +``` + +-------------------------------------------------------------------------------- + +### Cross-Domain Dependencies + +* **Upstream:** T6 | CLI Argument Parsing & UX Consistency - *The dynamic hook + injection framework fundamentally relies on standardized option parsers to + map CLI flags cleanly into hook runner contexts.*