What happened
PR #1543 added distributed tracing with timing span emission across integration-service controllers. The fullsend-ai-review bot ran 10 iterations, gemini-code-assist posted 5 suggestions, and CodeQL flagged one issue. Despite this automated coverage, the most critical bugs were found only by human reviewer kasemAlem on 2026-05-18: two missing emission paths in the Application model flow of buildpipeline_adapter.go. Specifically:
- The
EnsureSnapshotExists (ComponentGroup flow) had emission but EnsureSnapshotExistsApplication did not.
- An early return in
EnsureSnapshotExistsApplication when the snapshot annotation already exists skipped timing span emission entirely on restarts/requeues.
These were domain-specific completeness bugs that required knowledge of the dual-model architecture. No automated reviewer flagged them because the AGENTS.md file does not document the dual-model pattern as a completeness requirement.
What could go better
The repo's AGENTS.md documents the adapter pattern, controller structure, and development guidelines, but does not mention that integration-service maintains two parallel flows — ComponentGroup (new) and Application (legacy, planned for deprecation) — and that feature implementations in controllers must cover both. This context gap is the root cause of the automated review miss.
The dual-model pattern is visible in code (e.g., NewAdapter() vs NewAdapterWithApplication() in snapshot_adapter.go, TODO comments about deprecation) but is never stated as a completeness requirement. Human reviewers like kasemAlem know this from experience; automated reviewers need it spelled out.
Confidence: High. The evidence is clear — kasemAlem's comment explicitly says "if you want to continue with implementing this distributed tracing feature you should take care of both flows" and points to two specific locations. This is a repeatable pattern: any future cross-cutting feature (metrics, logging changes, new annotations) faces the same risk of incomplete dual-model coverage.
Proposed change
Add a "Dual-Model Architecture" section to AGENTS.md (which serves as the repo's CLAUDE.md) under the Architecture heading. The section should:
- Explain that integration-service currently supports two parallel flows: ComponentGroup model (new) and Application model (legacy, planned for deprecation per README.md).
- State explicitly that any feature touching controllers/adapters must be implemented and tested for both flows until the Application model is fully deprecated.
- List the key dual-path locations:
snapshot_adapter.go (NewAdapter vs NewAdapterWithApplication), buildpipeline_adapter.go (EnsureSnapshotExists vs EnsureSnapshotExistsApplication), and related test files.
- Note that TODO comments marked "remove when we deprecate old application model" indicate Application-model code paths that must be maintained.
Example addition after the "Adapter Pattern" subsection:
### Dual-Model Architecture
The service supports two parallel resource flows: **ComponentGroup** (new model) and **Application** (legacy, planned for deprecation). Controllers and adapters implement both flows side-by-side (e.g., `EnsureSnapshotExists` vs `EnsureSnapshotExistsApplication`). Any cross-cutting feature — tracing, metrics, annotations, error handling — must cover both flows until the Application model is fully removed. Look for `TODO: remove when we deprecate old application model` comments to identify Application-model code paths.
Validation criteria
- The next cross-cutting feature PR to integration-service (touching 2+ controllers) should have automated review findings that reference dual-model completeness, or should not require a human to catch missing coverage in one of the two flows.
- The AGENTS.md file contains an explicit mention of both ComponentGroup and Application flows as a completeness requirement.
- Measurable over the next 3 PRs that touch controller adapters: zero human-only catches of missing dual-model coverage that automated reviewers should have flagged.
Generated by retro agent from #1543
What happened
PR #1543 added distributed tracing with timing span emission across integration-service controllers. The fullsend-ai-review bot ran 10 iterations, gemini-code-assist posted 5 suggestions, and CodeQL flagged one issue. Despite this automated coverage, the most critical bugs were found only by human reviewer kasemAlem on 2026-05-18: two missing emission paths in the Application model flow of
buildpipeline_adapter.go. Specifically:EnsureSnapshotExists(ComponentGroup flow) had emission butEnsureSnapshotExistsApplicationdid not.EnsureSnapshotExistsApplicationwhen the snapshot annotation already exists skipped timing span emission entirely on restarts/requeues.These were domain-specific completeness bugs that required knowledge of the dual-model architecture. No automated reviewer flagged them because the AGENTS.md file does not document the dual-model pattern as a completeness requirement.
What could go better
The repo's AGENTS.md documents the adapter pattern, controller structure, and development guidelines, but does not mention that integration-service maintains two parallel flows — ComponentGroup (new) and Application (legacy, planned for deprecation) — and that feature implementations in controllers must cover both. This context gap is the root cause of the automated review miss.
The dual-model pattern is visible in code (e.g.,
NewAdapter()vsNewAdapterWithApplication()insnapshot_adapter.go, TODO comments about deprecation) but is never stated as a completeness requirement. Human reviewers like kasemAlem know this from experience; automated reviewers need it spelled out.Confidence: High. The evidence is clear — kasemAlem's comment explicitly says "if you want to continue with implementing this distributed tracing feature you should take care of both flows" and points to two specific locations. This is a repeatable pattern: any future cross-cutting feature (metrics, logging changes, new annotations) faces the same risk of incomplete dual-model coverage.
Proposed change
Add a "Dual-Model Architecture" section to
AGENTS.md(which serves as the repo's CLAUDE.md) under the Architecture heading. The section should:snapshot_adapter.go(NewAdaptervsNewAdapterWithApplication),buildpipeline_adapter.go(EnsureSnapshotExistsvsEnsureSnapshotExistsApplication), and related test files.Example addition after the "Adapter Pattern" subsection:
Validation criteria
Generated by retro agent from #1543