refactor(#249): add caller-callee contract consistency heuristic

[fullsend-ai-coder]

Add a review heuristic for detecting cross-function data flow inconsistencies in security-relevant struct fields. When a function locally defaults a security-relevant field (allowlist, ACL, permission set) without writing it back to the struct, the caller may pass the same struct to downstream functions that interpret nil/absent differently, creating a fail-open pattern.

Changes:

• security.md: new "Caller-callee contract consistency" section
  after "Fail-open / fail-closed evaluation" using fail-open
  category, with 4-step heuristic and edge-case exclusions
• code-review/SKILL.md: corresponding bullet in the Security
  dimension for standalone reviews

Note: pre-commit could not run in the sandbox (shellcheck install blocked by network restrictions). Changes are markdown-only.

---

Closes #249

Post-script verification

• Branch is not main/master (agent/249-caller-callee-security-heuristic)
• Secret scan passed (gitleaks — d8e3df7c018996e396c79b8ecd59e4145a628a6f..HEAD)
• Pre-commit hooks passed (authoritative run on runner)
• Tests ran inside sandbox

[github-actions]

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:42 PM UTC · Completed 4:52 PM UTC
Commit: d8e3df7 · View workflow run →

[fullsend-ai-review]

Review — comment

PR adds a caller-callee contract consistency heuristic to the review agent's security analysis, addressing issue #249. The heuristic is well-structured: it correctly models the two inconsistency directions, provides appropriate severity mappings, and lists reasonable edge-case exclusions. The SKILL.md bullet and security.md section are mutually consistent. No scope creep beyond the issue's authorization.

Findings

1. [medium] Internal category contradiction — security.md

File: internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security.md
Category: logic-error

The section header states:

Category: Use fail-open for all findings in this section.

But step 4 of the heuristic says the permit-then-deny direction should use category logic-error at medium severity. "All findings" and "except this one" are contradictory — a reviewer following the instructions mechanically would either use fail-open (per the header) or logic-error (per step 4), depending on which they read last.

Remediation: Change the category designation to acknowledge both categories. For example: "Category: Use fail-open for deny-then-permit findings; use logic-error for permit-then-deny findings (see step 4)."

2. [low] Commit type prefix — PR title

Category: tier-mismatch

The PR title uses refactor(#249) but this change is documentation-only (two markdown files, no code restructuring). Per COMMITS.md, refactor = "Code restructuring (no behavior change)" — this PR neither restructures code nor leaves behavior unchanged (it adds a new heuristic). The correct prefix is docs (documentation only) since the change consists entirely of markdown skill-file additions. COMMITS.md notes that miscategorization as refactor is "harmless" (wrong release-notes section rather than a semantic error), which is why this is low rather than medium.

3. [low] Missing edge-case exclusion — security.md

File: internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security.md
Category: logic-error

The three listed exclusions cover: (a) function writes default back, (b) struct not reused after the call, (c) field is explicitly empty (not nil). Missing is the case where the caller explicitly sets the field to a non-nil value between calls. The struct IS reused (so exclusion b does not apply), but the field is corrected before the next callee sees it, so no inconsistency exists. Without this exclusion, the heuristic could produce false positives on code that correctly patches the field between calls.

Remediation: Add a fourth exclusion: "The caller explicitly sets the field to a non-nil value between the two calls."

Dimensions reviewed

Dimension	Result
Correctness	1 medium (category contradiction), 1 low (missing exclusion)
Security	No findings (docs-only, no secrets/injection/permission changes)
Intent & coherence	1 low (commit prefix); authorization and scope confirmed
Style & conventions	No findings — heading style, bullet format, and severity notation match existing patterns
Docs currency	No findings — PR is the documentation change itself
Cross-repo contracts	Skipped — no exported interfaces modified

Previous run

Review — PR #283

refactor(#249): add caller-callee contract consistency heuristic

Summary

This PR adds a caller-callee contract consistency heuristic to the review agent's skill definitions, implementing the guidance from issue #249. The change correctly targets both the standalone code-review skill (SKILL.md) and the orchestrator's security sub-agent (security.md), following the established pattern where SKILL.md provides summary guidance and sub-agent files provide detailed procedures.

The heuristic itself is well-designed: it captures the real fail-open pattern described in issue #249 (the tryAgentsRepoFallback / LoadWithBase nil-vs-default asymmetry) and includes appropriate severity differentiation between deny-then-permit (fail-open, high) and permit-then-deny (functional bug, medium). The edge-case exclusions are sound and prevent false positives without creating bypass opportunities.

One internal consistency issue was found in the security.md additions.

Findings

1. [medium] Category directive contradicts step 4 — security.md

File: internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security.md
Category: logic-error

The new section opens with:

Category: Use fail-open for all findings in this section.

But step 4 explicitly instructs the agent to use a different category for one class of findings:

Permit-then-deny (first callee permits, subsequent callee defaults to deny-all): this is a service-disruption / functional correctness bug, not a security fail-open. Use category logic-error at medium severity.

These are contradictory directives. The blanket rule says fail-open for all findings; the specific rule says logic-error for permit-then-deny. An agent will receive conflicting instructions with no precedence guidance. In practice the more specific instruction would likely win, but the blanket directive creates unnecessary ambiguity and risk of miscategorization.

The corresponding bullet in SKILL.md does not have this issue — it lacks a blanket category directive and simply specifies categories inline.

Remediation: Update the section-level category directive to reflect both categories:

**Category:** Use `fail-open` for deny-then-permit findings; use
`logic-error` for permit-then-deny findings (see step 4).

Alternatively, remove the blanket directive and let step 4 be the sole authority on category assignment.

2. [low] Parenthetical category+severity guidance differs from established pattern — SKILL.md

File: internal/scaffold/fullsend-repo/skills/code-review/SKILL.md
Category: style-consistency

The new bullet embeds category assignment in a parenthetical — (use category \logic-error` at medium for the permit-then-deny direction, which is a functional correctness bug)` — while existing bullets in the Security dimension use declarative prose for severity guidance (e.g., "must be at least high severity"). The parenthetical form is functional but slightly harder to parse on a quick scan.

Observations

• Authorization: Change traces cleanly to issue [RTK+Ponytail] Review agent should verify caller-callee consistency for security-relevant defaults #249. Scope matches — no scope creep.
• Commit prefix: refactor is correct per COMMITS.md ("tightening internal heuristics, adjusting prompts" → refactor; see also the refactor(#1797): extract challenger pass into dedicated sub-agent example).
• Architecture: The dual-file approach (summary bullet in SKILL.md + detailed section in security.md) follows the established pattern used by existing sections (fail-open, permissions).
• Security: No injection patterns, secrets, Unicode steganography, or review-weakening guidance found. The edge-case exclusions are narrowly scoped and eliminate only genuine false positives.
• Cross-repo contracts: Not applicable — no API surface changes.

Previous run (2)

Review

Findings

Medium

• [logic-error] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security.md:140 — Step 4 defines a fail-open path as "where one function permits access while a subsequent function denies it, or vice versa." The explicitly stated direction (permit-then-deny) is a fail-closed / service-disruption pattern, not fail-open. The "or vice versa" direction (deny-then-permit) IS actually fail-open. Since the section header mandates "Category: Use fail-open for all findings in this section," both directions would be reported as fail-open, but only the deny-then-permit direction is a genuine fail-open vulnerability. The permit-then-deny direction is a functional correctness / availability bug. Conflating them under one category and severity could mislead triage.
  Remediation: Split the severity guidance — keep high with category fail-open for the deny-then-permit direction (actual privilege escalation risk). For the permit-then-deny direction (service disruption / deny-all), use a different category or clarify that it is a functional correctness issue at medium severity, not a security fail-open.

Low

• [edge-case] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security.md:149 — The edge-case exclusion for "explicitly empty value (e.g., empty slice, not nil)" relies on the nil-vs-empty-slice distinction, which is a Go idiom. In languages without this type-level distinction (Python None vs [], JavaScript null vs undefined), the heuristic may not map cleanly. Acceptable for the primary Go use case but may produce false negatives in polyglot reviews.

• [terminology-consistency] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security.md:107 — The section header "Caller-callee contract consistency" introduces terminology not used elsewhere in the skill files. However, "caller-callee contract" is a well-established software engineering term and is internally consistent across both changed files, so this is an observation rather than a required change.

---

Labels: PR adds a security review heuristic as markdown-only documentation changes.

[guyoron1]

/fs-fix

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 5:38 PM UTC · Completed 5:46 PM UTC
Commit: d8e3df7 · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 1 (human-triggered)

Fixed 1 of 3 findings. The medium logic-error (step 4 direction conflation) was valid and corrected in both security.md and code-review/SKILL.md. The two low findings were observations the reviewer explicitly marked as not requiring changes.

Fixed (1):

1. Step 4 conflates permit-then-deny (fail-closed) with deny-then-permit (fail-open) under one category (internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security.md): Split step 4 severity guidance: deny-then-permit direction kept as fail-open at high severity; permit-then-deny direction reclassified as logic-error at medium severity. Also updated the corresponding bullet in code-review/SKILL.md.

Disagreed (2):

1. nil-vs-empty-slice distinction is Go-specific, may not map to polyglot reviews: Reviewer explicitly noted this is acceptable for the primary Go use case. The heuristic targets Go struct semantics; adding multi-language caveats would over-scope this section.
2. Caller-callee contract terminology not used elsewhere in skill files: Reviewer explicitly stated this is an observation rather than a required change. The term is well-established in software engineering and internally consistent across both changed files.

Tests: passed

_{Updated by fullsend fix agent}

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:49 PM UTC · Completed 5:58 PM UTC
Commit: d8e3df7 · View workflow run →

[guyoron1]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:50 AM UTC · Completed 4:59 AM UTC
Commit: d8e3df7 · View workflow run →