Skip to content

docs(#6197): document Fn suffix convention for injectable function variables - #6200

Merged
rh-hemartin merged 1 commit into
mainfrom
agent/6197-doc-fn-suffix-convention
Aug 26, 2026
Merged

docs(#6197): document Fn suffix convention for injectable function variables#6200
rh-hemartin merged 1 commit into
mainfrom
agent/6197-doc-fn-suffix-convention

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

Summary

Documents the XxxFn suffix naming convention for injectable function variables (test seams) in docs/contributing/go-code.md. This convention has 9+ consistent instances across internal/sandbox/ and internal/dispatch/cf/ but was previously undocumented, causing agents to infer it from code patterns rather than referencing written guidance.

Changes

  • Added "Injectable function variables (test seams)" section to docs/contributing/go-code.md after the "Concurrency testing" section
  • Documents four rules: Fn suffix, default to real implementation, "Override in tests to..." doc comment pattern, and t.Cleanup restoration
  • References real examples (RetrySleepFn, BuildWASMFn, CopyWASMExecFn)

Testing

  • Documentation-only change — no Go code modified
  • Referenced example files verified to exist
  • Secret scan passed
  • Gitlint passed

Checklist

  • PR title follows Conventional Commits (correct type, ! for breaking changes)
  • I wrote this contribution myself and can explain all changes in it

Closes #6197

Post-script verification

  • Branch is not main/master (agent/6197-doc-fn-suffix-convention)
  • Secret scan passed (gitleaks — ed0deb850458a4eb2107b51298a569c7503295db..HEAD)
  • PR body secret scan passed (gitleaks — no-git)

@fullsend-ai-coder
fullsend-ai-coder Bot requested a review from a team as a code owner August 13, 2026 18:06
@fullsend-ai-coder fullsend-ai-coder Bot added the ready-for-review Triggers review agent dispatch label Aug 13, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 13, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 6:07 PM UTC · Completed 6:19 PM UTC

Commit: 1365be4 · View workflow run →

@github-actions

github-actions Bot commented Aug 13, 2026

Copy link
Copy Markdown

Site preview

Preview: https://910b3f06-site.fullsend-ai.workers.dev

Commit: 7467e9656607a637e820666d2e3d85d041484cfb

@codecov

codecov Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 13, 2026

Copy link
Copy Markdown

Review

Findings

Medium

  • [accuracy] docs/contributing/go-code.md:258 — The doc comment requirement says comments must "start with 'Override in tests to...'" but every cited example in the codebase follows standard Go doc-comment convention: they start with the variable name, then include "Override in tests to..." as a later sentence. For example, RetrySleepFn's comment is: "RetrySleepFn is the function called between retry attempts in CreateWithRetry. It defaults to time.Sleep. Override in tests to avoid real delays." The phrasing "starting with" is inaccurate relative to the codebase's own examples and contradicts Go doc-comment convention (godoc expects the first word to be the declared name).
    Remediation: Change "Include a doc comment starting with 'Override in tests to...'" to "Include a doc comment following Go convention (starting with the variable name) that contains an 'Override in tests to...' sentence describing what the override achieves."

Low

  • [accuracy] docs/contributing/go-code.md:255 — The documentation states the convention uses an XxxFn suffix (capitalized/exported), but the codebase also has unexported test seams following the same Fn-suffix pattern: resolveOverrideFn in internal/sandbox/gateway_endpoint.go and execCombinedOutputFn in internal/dispatch/cf/provisioner.go. The documented convention does not acknowledge that unexported (xxxFn) variables also exist and follow the same pattern.
    Remediation: Either note that the XxxFn convention applies to exported test seams and that unexported seams use the same xxxFn suffix in lowercase, or broaden the rule to cover both cases.
Previous run

Review

Findings

Medium

  • [accuracy] docs/contributing/go-code.md:165 — The doc comment requirement says comments must "start with 'Override in tests to...'" but every cited example in the codebase follows a different pattern: they start with the variable name per Go convention, then include "Override in tests to..." as a later sentence. For example, RetrySleepFn's comment is: "RetrySleepFn is the function called between retry attempts in CreateWithRetry. It defaults to time.Sleep. Override in tests to avoid real delays." The phrasing "starting with" is inaccurate relative to the codebase's own examples.
    Remediation: Change "Include a doc comment starting with 'Override in tests to...'" to "Include a doc comment following Go convention (starting with the variable name) that contains an 'Override in tests to...' sentence describing the override behavior."

Low

  • [stale-reference] docs/contributing/go-code.md:163 — The documentation states that injectable function variables must use an XxxFn suffix (capitalized/exported), but internal/sandbox/gateway_endpoint.go has resolveOverrideFn (lowercase/unexported) that also serves as a test seam following the same pattern. The documented convention does not account for unexported test seams.
    Remediation: Add a note that this convention applies to exported/newly-introduced test seams, or clarify that lowercase xxxFn variables are a legacy pattern.

Labels: PR modifies documentation under docs/contributing/ about Go code conventions

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment documentation component/docs User-facing documentation labels Aug 13, 2026
@rh-hemartin

Copy link
Copy Markdown
Member

/fs-fix rebase and resolve conflicts

@fullsend-ai-coder

fullsend-ai-coder Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ❌ Failure (post-script /home/runner/work/fullsend/fullsend/.fullsend/.fullsend-cache/resources/sha256/1704f826be06352c5aa727ca7a68e6bc3db1837e9743e1727323664169eefe2b/scripts/post-fix.sh failed: exit status 1) · Started 8:45 AM UTC · Completed 8:50 AM UTC

Commit: 1365be4 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.54

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

⚠️ Post-fix script failed — Secret scan blocked (exit code 1)

The fix agent completed, but the post-fix script failed before finishing.

Workflow run: https://github.com/fullsend-ai/fullsend/actions/runs/32949230655

Details:
Secret scan blocked the push. See workflow logs for details.
Please check the workflow logs for full details and retry with /fs-fix if appropriate.

@rh-hemartin
rh-hemartin force-pushed the agent/6197-doc-fn-suffix-convention branch from 1365be4 to 7467e96 Compare August 26, 2026 10:11
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 26, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:13 AM UTC · Completed 10:29 AM UTC

Commit: 7467e96 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.58

@fullsend-ai-review fullsend-ai-review Bot added the risk/moderate PR risk: moderate label Aug 26, 2026
@fullsend-ai-review

Copy link
Copy Markdown

Risk Assessment: moderate (2/5)

Details

Minimal-risk documentation-only PR adding 11 lines to an existing contributing guide. Tier 2 slightly elevated due to high churn on go-code.md (21 commits in 30 days, 6 authors), but offset by trivial change size, bot author, and precise issue scope alignment.

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the review comment for full details.

Comment thread docs/contributing/go-code.md
Comment thread docs/contributing/go-code.md
…riables

Add an "Injectable function variables (test seams)" section to
docs/contributing/go-code.md documenting the naming and usage
convention for package-level function variables used as test
seams. The convention (XxxFn suffix, default to real impl, doc
comment pattern, t.Cleanup restoration) has 9+ instances with
zero violations but was previously undocumented, causing agents
to infer the pattern from code rather than referencing written
guidance.

Note: pre-commit could not run in sandbox (network restriction).
Manual verification of pre-commit is required.

Closes #6197
@rh-hemartin
rh-hemartin force-pushed the agent/6197-doc-fn-suffix-convention branch from 7467e96 to d06fe96 Compare August 26, 2026 13:14
@rh-hemartin
rh-hemartin added this pull request to the merge queue Aug 26, 2026
Merged via the queue into main with commit d0d567b Aug 26, 2026
20 checks passed
@rh-hemartin
rh-hemartin deleted the agent/6197-doc-fn-suffix-convention branch August 26, 2026 13:16
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 26, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 1:16 PM UTC · Completed 1:31 PM UTC

Commit: d06fe96 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.12

@fullsend-ai-review

Copy link
Copy Markdown

Review skipped — this PR is already merged.

The /fs-review command only reviews open PRs/MRs.

Posted by fullsend pre-review check

@fullsend-ai-retro

fullsend-ai-retro Bot commented Aug 26, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 1:17 PM UTC · Completed 1:29 PM UTC

Commit: d06fe96 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.13

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #6200 — Document Fn suffix convention

Timeline

  1. Aug 13 — Retro agent filed #6197 from the PR #6066 retro, proposing to document the Fn suffix convention for injectable function variables. The issue included example documentation content.
  2. Aug 13 — Code agent implemented the proposed content in PR #6200 (11 lines added to docs/contributing/go-code.md, ~5 min, 1 commit).
  3. Aug 13 — Review agent found 2 accuracy issues: (a) the doc comment rule says "starting with 'Override in tests to...'" but codebase examples start with the variable name per Go convention; (b) only covers exported XxxFn but codebase also has unexported xxxFn variants.
  4. Aug 26 (13 days later) — Human reviewer rh-hemartin requested /fs-fix rebase and resolve conflicts.
  5. Aug 26 — Fix agent rebased correctly but post-script gitleaks scan failed — scanned 522 commits including upstream main, found 6 pre-existing leaks (false positives). Cost: $1.54.
  6. Aug 26 — Human approved and merged. Second review agent run posted identical findings ($3.58). PR merged without addressing either accuracy issue.

Key findings

Documentation merged with known inaccuracies. The review agent correctly identified that the doc comment guidance is inaccurate (says comments should start with "Override in tests to..." but Go convention and all codebase examples start with the variable name). The review also noted the documentation omits unexported xxxFn variants. Neither finding was addressed before merge. A proposal is filed below.

Evidence for existing issues:

  • #6259: The fix agent gitleaks failure is an exact instance of this bug — post-fix.sh scanned 522 commits after rebase, including all upstream main history, finding 6 pre-existing leaks that blocked the push.
  • #2959: The review agent posted identical findings on both runs (Aug 13 and Aug 26) without deduplication. The second review cost $3.58 and added no new information.
  • #6018: The review agent posted findings as COMMENTED (not CHANGES_REQUESTED) despite the requires-manual-review label being applied. Had changes been requested, the human would have needed to explicitly dismiss the review to merge.

Autonomy observation

The review agent's accuracy findings were correct and caught issues the human review did not address. This supports the case for the review agent having stronger gating authority on accuracy-class findings for documentation PRs — medium-severity accuracy findings could block merge rather than posting as informational comments. This aligns with #6018.

Proposals filed

@fullsend-ai-review

Copy link
Copy Markdown

Review skipped — this PR is already merged.

The /fs-review command only reviews open PRs/MRs.

Posted by fullsend post-review check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/docs User-facing documentation documentation ready-for-review Triggers review agent dispatch requires-manual-review Review requires human judgment risk/moderate PR risk: moderate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Document Fn suffix convention for injectable function variables in go-code.md

1 participant