What happened
PR #2947 added tryAgentsRepoFallback which defaults a nil OrgAllowlist to config.DefaultAllowedRemoteResources() for its own fetch (run.go:2729-2731). The review agent flagged this as [low] fail-open with 'Mitigated: an explicitly empty allowlist is not nil, so the default is NOT applied.' This assessment was correct for the function in isolation. However, the agent did not trace the data flow back to the caller runAgent, where the same composeOpts (still with nil OrgAllowlist) was passed to LoadWithBase, which applied deny-all semantics for nil. This asymmetry — function defaults to allow internally, caller's next operation defaults to deny — caused #3396, breaking all 4 triage eval cases in functional-tests on main for ~18 hours after merge. Three human reviewers (ralphbean, waynesun09, ifireball) and two other review bots (qodo, review squad) also missed this across 10+ review cycles.
What could go better
The review agent should trace cross-function data flow when a function applies a local default for a security-relevant field. When a callee does localVar := structParam.Field; if localVar == nil { localVar = default } and uses localVar internally without updating structParam.Field, the agent should check whether the caller passes the same struct to other functions that depend on the field. This is especially critical for security boundaries (allowlists, ACLs, permission sets) where nil semantics differ between deny-all and use-defaults depending on context. The review agent was close to catching this — it identified the defaulting behavior — but stopped analysis at the function boundary instead of following the struct through the caller's subsequent operations. Confidence: high — the data flow is only 3 hops (tryAgentsRepoFallback → runAgent → LoadWithBase) and all in the same file.
Proposed change
Add a review heuristic to the review agent's security-sensitive code analysis: when a function applies a local default for a security-relevant field from a struct parameter without updating the struct, flag it as a potential caller-callee contract inconsistency. The heuristic: 'If a function receives a security-relevant field via a struct parameter and applies a permissive default (e.g., fallback allowlist) for internal use without writing it back to the struct, verify that callers do not pass the same struct to downstream functions that interpret nil/absent as deny-all.' This is related to #3417 (fail-open patterns) and #3405 (config propagation gaps) but covers a distinct pattern: asymmetric default interpretation between caller and callee within the same code path.
Validation criteria
On a future PR that adds a function which locally defaults a security-relevant field from a struct parameter without writing it back, the review agent should flag the potential inconsistency. Specifically: (1) the review agent mentions the caller-callee contract risk, not just the internal behavior; (2) severity is at least MEDIUM (not low/mitigated) when the field controls a security boundary. Can be validated by constructing a synthetic test case with this pattern and running the review agent against it.
Generated by retro agent from fullsend-ai#3425
What happened
PR #2947 added
tryAgentsRepoFallbackwhich defaults a nilOrgAllowlisttoconfig.DefaultAllowedRemoteResources()for its own fetch (run.go:2729-2731). The review agent flagged this as[low] fail-openwith 'Mitigated: an explicitly empty allowlist is not nil, so the default is NOT applied.' This assessment was correct for the function in isolation. However, the agent did not trace the data flow back to the callerrunAgent, where the samecomposeOpts(still with nilOrgAllowlist) was passed toLoadWithBase, which applied deny-all semantics for nil. This asymmetry — function defaults to allow internally, caller's next operation defaults to deny — caused #3396, breaking all 4 triage eval cases in functional-tests on main for ~18 hours after merge. Three human reviewers (ralphbean, waynesun09, ifireball) and two other review bots (qodo, review squad) also missed this across 10+ review cycles.What could go better
The review agent should trace cross-function data flow when a function applies a local default for a security-relevant field. When a callee does
localVar := structParam.Field; if localVar == nil { localVar = default }and useslocalVarinternally without updatingstructParam.Field, the agent should check whether the caller passes the same struct to other functions that depend on the field. This is especially critical for security boundaries (allowlists, ACLs, permission sets) where nil semantics differ between deny-all and use-defaults depending on context. The review agent was close to catching this — it identified the defaulting behavior — but stopped analysis at the function boundary instead of following the struct through the caller's subsequent operations. Confidence: high — the data flow is only 3 hops (tryAgentsRepoFallback → runAgent → LoadWithBase) and all in the same file.Proposed change
Add a review heuristic to the review agent's security-sensitive code analysis: when a function applies a local default for a security-relevant field from a struct parameter without updating the struct, flag it as a potential caller-callee contract inconsistency. The heuristic: 'If a function receives a security-relevant field via a struct parameter and applies a permissive default (e.g., fallback allowlist) for internal use without writing it back to the struct, verify that callers do not pass the same struct to downstream functions that interpret nil/absent as deny-all.' This is related to #3417 (fail-open patterns) and #3405 (config propagation gaps) but covers a distinct pattern: asymmetric default interpretation between caller and callee within the same code path.
Validation criteria
On a future PR that adds a function which locally defaults a security-relevant field from a struct parameter without writing it back, the review agent should flag the potential inconsistency. Specifically: (1) the review agent mentions the caller-callee contract risk, not just the internal behavior; (2) severity is at least MEDIUM (not low/mitigated) when the field controls a security boundary. Can be validated by constructing a synthetic test case with this pattern and running the review agent against it.
Generated by retro agent from fullsend-ai#3425