docs(architecture): clarify AI agent repository guide by sbalabanov · Pull Request #270 · uber/tango

sbalabanov · 2026-08-03T23:40:37Z

Summary

Intent:

document Tango-specific identity, ownership, caching, and extension boundaries accurately
keep the documentation update as a focused, reviewable CLAUDE.md diff

Changes:

restructured the existing repository guide around component ownership and artifact-specific cache behavior
clarified entity handoffs, extensibility, error classification, cancellation, and validation guidance

Test Plan

reviewed the merge-base diff and confirmed it modifies only CLAUDE.md
verified the Markdown structure and documented repository paths locally

Revert Plan

revert this documentation commit

_{Generated by the 🪄 pr-update skill in devexp-agent-marketplace}

Issues

CLAassistant · 2026-08-03T23:40:44Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

sbalabanov · 2026-08-04T00:51:33Z

-### Controllers
-
-The controller is the YARPC service implementation. It owns the transport-adjacent concerns: request validation, response chunking, cancellation handling, fan-out across revisions, and metrics emission. It does **not** own workspace creation, git operations, or graph computation — those belong to the orchestrator and below.
+### Responsibility and artifact ownership


remove this section

Removed the responsibility and artifact ownership section in b84dc9f. The architecture guidance now stays concise and relies on the component sections instead of a detailed ownership matrix.

[addressed by agent]

sbalabanov · 2026-08-04T00:52:25Z

+Every RPC records operation metrics, classifies invalid input as a user error, preserves error chains, and converts the final error at the wire boundary. The graph RPC and comparison RPC have distinct cache flows:

-**Most production monorepo setups will need to provide their own `Orchestrator` implementation** that integrates with the host CI system (e.g. Buildkite, internal build infrastructure) instead of managing local clones. CI-driven environments already own:
+- **`GetTargetGraph`** validates and maps the request, attempts the treehash/graph fast path, delegates a miss to the orchestrator, then streams the returned reader.


do not list existing implementations, keep the guide generic

Removed the named RPC flow list in b84dc9f. The controller description now states the generic request responsibilities without enumerating current RPC implementations.

[addressed by agent]

sbalabanov · 2026-08-04T00:54:46Z

+### Orchestrator

-A custom orchestrator satisfies the same `orchestrator.Orchestrator` interface and is wired into the controller in place of the native one. It is the right seam to plug in remote build execution, CI-managed checkouts, or organization-specific caching — the controller and `graphrunner` stay unchanged.
+The orchestrator is the repository-identity resolution and workspace-materialization boundary. Given a `BuildDescription`, it produces a target graph by resolving the repository state, leasing and preparing a workspace, deriving its treehash, consulting the graph cache, invoking a graph runner on a miss, and synchronously storing the graph and treehash mapping.


simplify the definition - Orchestrator provides the target graph, typically by running Bazel. The implementation can do it at the local box or delegate to another stateful subsystem like CI (i.e. buildkite) that can manage checkout.

Simplified the Orchestrator definition in b84dc9f. It now says the orchestrator provides the target graph, typically through Bazel, either locally or by delegating checkout and computation to stateful CI infrastructure.

[addressed by agent]

sbalabanov · 2026-08-04T00:56:48Z

-New strategies plug in by satisfying the same interface. Keep them strategy-agnostic about how the workspace was assembled and what will be done with the result.
+1. **Describe data, not choreography** — comments state what a type or field means, its units, optionality, identity, and invariants. Component ownership and write paths belong in the architecture sections.
+2. **Prefer values for identities and configuration** — use value structs for `BuildDescription`, requests, configs, and constructor params. Pointers are appropriate for optional payloads, mutation, or shared ownership.
+3. **Treat slices and maps as mutable** — copy them before sorting, normalization, mutation, or concurrent handoff when the caller may retain or modify the original.


no need to mention that, it is Go's default

Removed the slices and maps mutability point in b84dc9f because it restated normal Go behavior rather than a Tango-specific rule.

[addressed by agent]

sbalabanov · 2026-08-04T00:57:31Z

+1. **Describe data, not choreography** — comments state what a type or field means, its units, optionality, identity, and invariants. Component ownership and write paths belong in the architecture sections.
+2. **Prefer values for identities and configuration** — use value structs for `BuildDescription`, requests, configs, and constructor params. Pointers are appropriate for optional payloads, mutation, or shared ownership.
+3. **Treat slices and maps as mutable** — copy them before sorting, normalization, mutation, or concurrent handoff when the caller may retain or modify the original.
+4. **Scope ID references to compact representations** — `OptimizedTarget` uses IDs and metadata maps for streamed and stored payloads; do not require every domain relationship to use IDs.


remove this point, it is not important enough where to ID and where to not

Removed the ID-reference placement point in b84dc9f to keep the entity guidance focused on higher-value repository conventions.

[addressed by agent]

sbalabanov · 2026-08-04T00:58:31Z

 - `core/workspace.Workspace` + `core/workspace.Request` — checkout, apply
 - `graphrunner.GraphRunner` — compute a graph from a workspace
- `orchestrator.Orchestrator` — top-level entry point
+- `orchestrator.Orchestrator` — repository identity to target graph


do not change, retain previous description

Restored the Orchestrator interface description to "top-level entry point" in b84dc9f.

[addressed by agent]

sbalabanov · 2026-08-04T01:00:40Z

 If you modified `.proto` files or interface signatures, also run `make proto` and regenerate the relevant mocks.

-**Commit and PR titles must follow the [Conventional Commits](https://www.conventionalcommits.org/) specification.** Use a type prefix (`feat`, `fix`, `docs`, `refactor`, `test`, `chore`, `build`, `ci`, `perf`, `style`) followed by an optional scope and a short imperative subject — e.g. `feat(orchestrator): support remote build execution`, `fix(controller): surface readTreehash errors`, `docs: add CLAUDE.md`. Breaking changes use `!` after the type/scope (e.g. `feat(storage)!: ...`) and explain the break in the body. This keeps the commit history machine-parseable for changelogs and release automation.
+**Commit and PR titles must follow the [Conventional Commits](https://www.conventionalcommits.org/) specification.** Use a type prefix (`feat`, `fix`, `docs`, `refactor`, `test`, `chore`, `build`, `ci`, `perf`, `style`) followed by an optional scope and a short imperative subject — e.g. `feat(orchestrator): support remote build execution`, `fix(controller): surface readTreehash errors`, `docs: update AGENTS.md`. Breaking changes use `!` after the type/scope (e.g. `feat(storage)!: ...`) and explain the break in the body. This keeps the commit history machine-parseable for changelogs and release automation.


instead of "followed by an optional scope" write "followed by scope"

Updated the Conventional Commits guidance in b84dc9f to require a scope and changed the documentation example accordingly.

[addressed by agent]

sbalabanov · 2026-08-04T01:03:54Z

+
+1. **Use `core/cachekey` helpers exclusively.** Never construct cache paths inline. When a new input changes computed graph or comparison output, update or version the relevant key and add tests. Current keys do not encode every repository configuration or algorithm/schema version, so such changes require deliberate compatibility or invalidation decisions.
+2. **Keep output-only options outside cache identities.** Store full graph and comparison payloads, then apply distance and field filtering while sending so presentation choices cannot poison shared entries.
+3. **Distinguish misses from failures.** A not-found result is an expected cache miss. Infrastructure failures on identity-bearing treehash or graph reads are fail-fast; compared-target cache reads and writes are best-effort and fall back to recomputation or logging.


change it to prefer to fail fast when either cache truly failed with infra error

Updated the cache guidance in b84dc9f to prefer failing fast when either target-graph or compared-target cache access has a genuine infrastructure failure, while retaining recomputation for misses or corrupt entries.

[addressed by agent]

sbalabanov · 2026-08-04T01:04:35Z

+2. **Keep output-only options outside cache identities.** Store full graph and comparison payloads, then apply distance and field filtering while sending so presentation choices cannot poison shared entries.
+3. **Distinguish misses from failures.** A not-found result is an expected cache miss. Infrastructure failures on identity-bearing treehash or graph reads are fail-fast; compared-target cache reads and writes are best-effort and fall back to recomputation or logging.
+4. **Expect duplicate work on concurrent misses.** Tango does not promise singleflight, compare-and-swap, or exactly-once computation. Deterministic computation plus complete content keys makes repeated overwrites converge on the same value.
+5. **Reject incomplete compared-target blobs before streaming.** Buffer cached comparison responses first; if decoding fails, discard the entry and recompute before sending any partial result.


remove this point (5)

Removed the standalone incomplete compared-target blob rule in b84dc9f and renumbered the remaining cache guidance.

[addressed by agent]

Summary: Intent: - document Tango-specific identity, ownership, caching, and extension boundaries accurately - keep the documentation update as a focused, reviewable CLAUDE.md diff Changes: - restructured the existing repository guide around component ownership and artifact-specific cache behavior - clarified entity handoffs, extensibility, error classification, cancellation, and validation guidance Test Plan: - reviewed the merge-base diff and confirmed it modifies only CLAUDE.md - verified the Markdown structure and documented repository paths locally Revert Plan: - revert this documentation commit --- <sub>Generated by the 🪄 [pr-update](https://sg.uberinternal.com/code.uber.internal/uber-code/devexp-agent-marketplace/-/blob/claude-code/plugins/dev/uber-dev/skills/pr-update/SKILL.md) skill in devexp-agent-marketplace</sub>

sbalabanov force-pushed the ai-docs branch from 5f32094 to 39f431d Compare August 4, 2026 00:01

sbalabanov changed the title ~~docs: add AI agent repository guide~~ docs: clarify AI agent repository guide Aug 4, 2026

sbalabanov commented Aug 4, 2026

View reviewed changes

sbalabanov changed the title ~~docs: clarify AI agent repository guide~~ docs(architecture): clarify AI agent repository guide Aug 4, 2026

sbalabanov marked this pull request as ready for review August 4, 2026 01:13

sbalabanov requested review from a team as code owners August 4, 2026 01:13

sbalabanov force-pushed the ai-docs branch from b84dc9f to 6d37348 Compare August 5, 2026 00:05

sbalabanov-zz added 2 commits August 5, 2026 00:06

docs(architecture): address review feedback

aaf6e70

sbalabanov force-pushed the ai-docs branch from 6d37348 to aaf6e70 Compare August 5, 2026 00:07

xytan0056 approved these changes Aug 5, 2026

View reviewed changes

yushan8 approved these changes Aug 5, 2026

View reviewed changes

sbalabanov merged commit 7358cd6 into main Aug 5, 2026
9 of 10 checks passed

Uh oh!

Conversation

sbalabanov commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Revert Plan

Issues

Uh oh!

CLAassistant commented Aug 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sbalabanov commented Aug 3, 2026 •

edited

Loading