What happened
On PR #1362, the review agent (fullsend-ai-review) approved the PR after finding 14 issues (all technically accurate, zero false positives). Human reviewer lipoja raised 3 concerns. Two were matched by agent findings: dead code in cleanup_on_failure (agent flagged as low/maintainability) and AI planning docs in commit history (agent flagged as medium/scope). However, the agent entirely missed the third: lipoja asked whether iib_environment_name should be constrained to a known set of environments rather than accepting any regex-valid string (comment). The agent praised the regex validation as well-designed without considering whether operationally invalid values should be caught at config time.
Additionally, the agent treated the dead code path in cleanup_on_failure as simple cleanup (low severity), while the human asked the deeper design question of whether the safety-net revert logic should remain functional — a distinction between "remove dead code" and "evaluate fault tolerance guarantees."
What could go better
The root cause is missing domain context. The agent has no way to know which Konflux environments exist (qe, stage, prod) or that a typo in the environment name would route to a nonexistent tenant. The repo has no .claude/ directory, no AGENTS.md, and no documented review conventions that would help the agent reason about operational semantics rather than just syntactic correctness.
The dead code under-weighting has a similar root cause: without guidance about the codebase's architectural transitions (direct-push to MR-based flow), the agent cannot distinguish "dead code to clean up" from "safety net during transition period."
Confidence: High that adding domain context would close the environment validation gap. Medium confidence on the dead code gap — the agent might still default to "remove dead code" even with guidance, since it requires judgment about transition risk.
This is not a one-off: any future PR modifying config validation or build handler ordering in this repo would face the same agent blind spot without domain context.
Proposed change
Create .claude/AGENTS.md in the release-engineering/iib repository with domain-specific review guidance covering at least:
-
Config validation: When reviewing config validation changes, check whether validated values correspond to real deployment targets. The system deploys to known Konflux environments; a regex-valid but operationally invalid environment name would fail late in the pipeline rather than at config load time. Reviewers should consider whether a warning for unexpected values would aid operators.
-
Git/registry consistency: The overwrite flow has a critical ordering constraint — all side effects that can fail (image replication, index.db push) must complete before the MR merge, because a merged MR cannot be reverted. Any PR that changes build handler ordering must be reviewed for git/registry divergence risk.
-
Architectural transitions: Some code paths are kept as safety nets during the transition from direct-push to unified MR flow. When flagging dead code, consider whether it serves as a fallback and whether removing it reduces fault tolerance during the transition.
-
Test conventions: Tests should validate both overwrite (merge MR after build) and throw-away (close MR after build) flows for each build handler type.
Validation criteria
On the next 3 PRs reviewed by fullsend-ai-review in release-engineering/iib that touch config validation or build handler logic: (1) the review agent should reference domain guidance when evaluating config constraints, (2) the agent should distinguish safety-net code from simple dead code in its findings, and (3) the gap rate against human review findings should drop below 20% (from 33% on this PR). Timeframe: within 3 months of AGENTS.md being merged.
Generated by retro agent from #1362
What happened
On PR #1362, the review agent (fullsend-ai-review) approved the PR after finding 14 issues (all technically accurate, zero false positives). Human reviewer lipoja raised 3 concerns. Two were matched by agent findings: dead code in cleanup_on_failure (agent flagged as low/maintainability) and AI planning docs in commit history (agent flagged as medium/scope). However, the agent entirely missed the third: lipoja asked whether
iib_environment_nameshould be constrained to a known set of environments rather than accepting any regex-valid string (comment). The agent praised the regex validation as well-designed without considering whether operationally invalid values should be caught at config time.Additionally, the agent treated the dead code path in
cleanup_on_failureas simple cleanup (low severity), while the human asked the deeper design question of whether the safety-net revert logic should remain functional — a distinction between "remove dead code" and "evaluate fault tolerance guarantees."What could go better
The root cause is missing domain context. The agent has no way to know which Konflux environments exist (qe, stage, prod) or that a typo in the environment name would route to a nonexistent tenant. The repo has no
.claude/directory, no AGENTS.md, and no documented review conventions that would help the agent reason about operational semantics rather than just syntactic correctness.The dead code under-weighting has a similar root cause: without guidance about the codebase's architectural transitions (direct-push to MR-based flow), the agent cannot distinguish "dead code to clean up" from "safety net during transition period."
Confidence: High that adding domain context would close the environment validation gap. Medium confidence on the dead code gap — the agent might still default to "remove dead code" even with guidance, since it requires judgment about transition risk.
This is not a one-off: any future PR modifying config validation or build handler ordering in this repo would face the same agent blind spot without domain context.
Proposed change
Create
.claude/AGENTS.mdin the release-engineering/iib repository with domain-specific review guidance covering at least:Config validation: When reviewing config validation changes, check whether validated values correspond to real deployment targets. The system deploys to known Konflux environments; a regex-valid but operationally invalid environment name would fail late in the pipeline rather than at config load time. Reviewers should consider whether a warning for unexpected values would aid operators.
Git/registry consistency: The overwrite flow has a critical ordering constraint — all side effects that can fail (image replication, index.db push) must complete before the MR merge, because a merged MR cannot be reverted. Any PR that changes build handler ordering must be reviewed for git/registry divergence risk.
Architectural transitions: Some code paths are kept as safety nets during the transition from direct-push to unified MR flow. When flagging dead code, consider whether it serves as a fallback and whether removing it reduces fault tolerance during the transition.
Test conventions: Tests should validate both overwrite (merge MR after build) and throw-away (close MR after build) flows for each build handler type.
Validation criteria
On the next 3 PRs reviewed by fullsend-ai-review in release-engineering/iib that touch config validation or build handler logic: (1) the review agent should reference domain guidance when evaluating config constraints, (2) the agent should distinguish safety-net code from simple dead code in its findings, and (3) the gap rate against human review findings should drop below 20% (from 33% on this PR). Timeframe: within 3 months of AGENTS.md being merged.
Generated by retro agent from #1362