Skip to content

docs: add Go developer cookbook for contributors - #1995

Closed
waynesun09 wants to merge 3 commits into
fullsend-ai:mainfrom
waynesun09:dev-cookbook
Closed

docs: add Go developer cookbook for contributors#1995
waynesun09 wants to merge 3 commits into
fullsend-ai:mainfrom
waynesun09:dev-cookbook

Conversation

@waynesun09

Copy link
Copy Markdown
Member

Summary

  • Adds a comprehensive Go developer cookbook at docs/guides/dev/go-cookbook.md
  • Covers project layout, Go conventions (error handling, interfaces, DI, Cobra CLI), testing patterns (testify, fakes, httptest)
  • Deep-dives into mint, inference, forge, and GitHub reusable workflow systems
  • Adds cookbook link to the guides README index

Test plan

  • Verify all internal links resolve correctly
  • Confirm code examples match current codebase patterns
  • Lint passes (make lint)

Comprehensive guide covering project layout, Go conventions (error
handling, interfaces, DI, Cobra CLI), testing patterns (testify, fakes,
httptest), and deep-dives into the mint, inference, forge, and GitHub
reusable workflow systems.

Signed-off-by: Wayne Sun <gsun@redhat.com>
@github-actions

github-actions Bot commented Jun 7, 2026

Copy link
Copy Markdown

Site preview

Preview: https://0f003a77-site.fullsend-ai.workers.dev

Commit: 8888242be4df3948d577ba9d56d086ee650fefc6

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 7, 2026

Copy link
Copy Markdown

Review

Findings

Medium

  • [missing-authorization] docs/guides/dev/go-cookbook.md — Non-trivial documentation addition (1492 lines, 9 major sections) without a linked issue. While the content has been verified as technically accurate, substantial additions benefit from a tracking issue to record the motivation, scope decisions (single cookbook vs. topic-specific guides), and relationship to existing dev guides.
    Remediation: Create a GitHub issue describing the rationale for a consolidated cookbook format and link it to this PR.

Low

  • [scope-coherence] docs/guides/dev/go-cookbook.md — The cookbook bundles 9 major topics (project layout, Go conventions, testing, mint, inference, forge, sandbox, security scanner, workflows) into a single 1492-line file. The existing dev guides follow a focused pattern (local-dev.md: environment setup, cli-internals.md: CLI architecture, testing-workflows.md: workflow testing). Consider whether some sections (e.g., mint system, security scanner) would be better served as standalone deep-dive guides that can evolve independently.

  • [architectural-alignment] docs/guides/dev/go-cookbook.md — ADR-0023 establishes writing rules including "one audience, one task" and "link, don't restate." While these rules primarily target admin/user guides, the cookbook's coverage of 9 subsystems in one file is worth evaluating against these principles. The existing cli-internals.md sets precedent for multi-topic dev guides, so this is not a violation but a design choice worth documenting.

  • [naming-convention] docs/guides/dev/go-cookbook.md:3 — Introduction uses "A practical guide for..." while the three existing dev guides consistently use "This guide [verb]..." (local-dev.md: "This guide walks through", cli-internals.md: "This guide provides", testing-workflows.md: "This guide explains").

  • [naming-convention] docs/guides/README.md:37 — The index entry description ("Go patterns, mint, inference, forge, and workflow internals for contributors") omits several major cookbook sections (testing patterns, sandbox system, security scanner). Consider aligning the description with the cookbook's actual breadth.

Previous run

Review

Findings

Low

  • [api-contract] docs/guides/dev/go-cookbook.md:669 — The forge.Client method table omits 11 of 54 methods across several categories: Issues is missing UpdateIssueComment and MinimizeComment; no row for PR file operations (GetPullRequestHeadSHA, ListPullRequestFiles, ListPullRequestFileDiffs); Workflows is missing GetWorkflowRun, GetWorkflowRunLogs, GetWorkflowRunAnnotations; Org Secrets is missing GetOrgSecretRepos; Org Variables is missing SetOrgVariableRepos and GetOrgVariableRepos. The doc does note "See internal/forge/forge.go for the complete interface" which mitigates this, but the tabular format implies more coverage than it provides.

  • [stale-doc] docs/guides/dev/go-cookbook.md — The cookbook restates rules from CLAUDE.md (forge abstraction requirement at line 714, testing commands) and AGENTS.md without cross-referencing the authoritative source. Only the sync requirement (line 589) references CLAUDE.md. Per ADR-0023 rule Use AI to help formalise intent after rapid local prototyping #4 ("Link, don't restate"), restated rules should reference their source to prevent future drift.

  • [naming-convention] docs/guides/dev/go-cookbook.md:3 — Introduction uses "A practical guide for..." while all three other dev/ guides consistently use "This guide [verb]..." (local-dev.md: "This guide walks through", cli-internals.md: "This guide provides", testing-workflows.md: "This guide explains").

  • [naming-convention] docs/guides/README.md:37 — The cookbook entry description ("Go patterns, mint, inference, forge, and workflow internals for contributors") does not match the document's actual introduction ("project conventions, key patterns, and deep-dives into the mint, inference, GitHub forge, and reusable workflow systems").

Info

  • [missing-authorization] docs/guides/dev/go-cookbook.md — Substantial 978-line new document with no linked issue. While pure documentation additions have lower authorization requirements than code changes, linking to an issue helps track the decision to add this content and provides context for future maintainers.
Previous run (2)

Review

Findings

High

  • [api-contract] docs/guides/dev/go-cookbook.md:534 — The canonicalRolePermissions table is materially incorrect across all 7 roles. Every role is missing "metadata": "read", and many roles are missing other permissions that exist in the actual code at internal/mintcore/github.go:57-64.

    Specific discrepancies:

    • triage: cookbook adds "pull_requests": "read" (does not exist); missing "metadata": "read"
    • coder: missing "checks": "read", "metadata": "read"
    • review: missing "issues": "write", "checks": "read", "metadata": "read"
    • fix: missing "metadata": "read"
    • retro: missing "pull_requests": "write", "metadata": "read"
    • prioritize: missing "organization_projects": "write", "metadata": "read"
    • fullsend: missing "actions_variables": "read", "pull_requests": "write", "metadata": "read"

    Remediation: Copy the actual canonicalRolePermissions map from internal/mintcore/github.go:57-64 into the cookbook verbatim.

Medium

  • [api-contract] docs/guides/dev/go-cookbook.md:753 — The CommitFiles method signature includes a branch parameter that does not exist in the actual interface. Cookbook shows: CommitFiles(ctx, owner, repo, branch, message string, files []TreeFile). Actual signature at internal/forge/forge.go:168: CommitFiles(ctx, owner, repo, message string, files []TreeFile). This would mislead contributors attempting to use the method.

    Remediation: Remove the branch parameter from the CommitFiles signature in the cookbook.

Low

  • [api-contract] docs/guides/dev/go-cookbook.md:712 — The forge.Client interface method table omits several method categories present in the actual interface (internal/forge/forge.go:128-269): org-level secrets/variables (5 methods each), workflow run logs/annotations, PR file diffs, org plan, app installations. While not claiming exhaustiveness, the tabular format implies a complete summary. Consider adding an "and others" note or expanding the table.

  • [stale-doc] docs/guides/dev/go-cookbook.md — The cookbook restates rules from CLAUDE.md (forge abstraction, mint sync requirements, testing requirements) and AGENTS.md (forge abstraction enforcement). While the sync requirement section (line 597) correctly references CLAUDE.md, other restated rules do not cross-reference the authoritative source, risking future drift.

    Remediation: Add cross-references to CLAUDE.md/AGENTS.md where rules are restated, or reference those documents instead of duplicating.

  • [naming-convention] docs/guides/dev/go-cookbook.md:3 — Introduction uses "A practical guide for..." while all other dev/ guides use the pattern "This guide [verb]..." (e.g., cli-internals.md: "This guide provides implementation details for...").

    Remediation: Change to "This guide covers Go conventions, testing patterns, and deep-dives into..." for consistency.

  • [naming-convention] docs/guides/dev/go-cookbook.md:17 — Section 6 in the Table of Contents is titled "The Forge Abstraction (GitHub Code Guide)" — the parenthetical is confusing since the forge abstraction is explicitly designed to be forge-agnostic (supporting GitHub, GitLab, Forgejo). The section content covers the general abstraction, not just the GitHub implementation.

    Remediation: Remove the parenthetical or change to "(Implementation Guide)".

Info

  • [missing-authorization] docs/guides/dev/go-cookbook.md — This is a substantial 983-line new document with no linked issue. While pure documentation additions have lower authorization requirements than code changes, linking to an issue helps track the decision to add this content and provides context for future maintainers.

@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/guides/dev/go-cookbook.md Outdated

```go
var canonicalRolePermissions = map[string]map[string]string{
"triage": {"contents": "read", "issues": "write", "pull_requests": "read"},

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[high] api-contract

The canonicalRolePermissions table is materially incorrect across all 7 roles. Every role is missing "metadata": "read", and many roles are missing other permissions that exist in the actual code at internal/mintcore/github.go:57-64. Specific discrepancies: triage adds "pull_requests": "read" (does not exist) and is missing "metadata": "read"; coder is missing "checks": "read" and "metadata": "read"; review is missing "issues": "write", "checks": "read", and "metadata": "read"; fix is missing "metadata": "read"; retro is missing "pull_requests": "write" and "metadata": "read"; prioritize is missing "organization_projects": "write" and "metadata": "read"; fullsend is missing "actions_variables": "read", "pull_requests": "write", and "metadata": "read".

Suggested fix: Copy the actual canonicalRolePermissions map from internal/mintcore/github.go:57-64 into the cookbook verbatim.

}

func (c *LiveClient) CommitFiles(ctx context.Context, owner, repo, branch, message string, files []TreeFile) (bool, error)
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[medium] api-contract

The CommitFiles method signature includes a branch parameter that does not exist in the actual interface. Cookbook shows: CommitFiles(ctx, owner, repo, branch, message string, files []TreeFile). Actual signature at internal/forge/forge.go:168: CommitFiles(ctx, owner, repo, message string, files []TreeFile).

Suggested fix: Remove the branch parameter from the CommitFiles signature in the cookbook.


| Category | Methods |
|----------|---------|
| **Repositories** | `ListOrgRepos`, `GetRepo`, `CreateRepo`, `DeleteRepo` |

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] api-contract

The forge.Client interface method table omits several method categories present in the actual interface (internal/forge/forge.go:128-269): org-level secrets/variables, workflow run logs/annotations, PR file diffs, org plan, app installations.

@@ -0,0 +1,983 @@
# Go Developer Cookbook

A practical guide for Go developers contributing to the fullsend codebase. Covers project conventions, key patterns, and deep-dives into the mint, inference, GitHub forge, and reusable workflow systems.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] naming-convention

Introduction uses 'A practical guide for...' while all other dev/ guides use the pattern 'This guide [verb]...'.

Comment thread docs/guides/dev/go-cookbook.md Outdated
4. [The Mint System](#the-mint-system)
5. [The Inference System](#the-inference-system)
6. [The Forge Abstraction (GitHub Code Guide)](#the-forge-abstraction)
7. [GitHub Reusable Workflows & Actions](#github-reusable-workflows--actions)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] naming-convention

Section 6 ToC title 'The Forge Abstraction (GitHub Code Guide)' parenthetical is confusing since the forge abstraction is designed to be forge-agnostic.

Fix code accuracy issues in the Go developer cookbook identified by
multi-agent review:

- Use regexp.MustCompile() for pattern variables (not raw strings)
- Combine ValidateOrgName into single conditional (matches source)
- Fix canonicalRolePermissions for all 7 roles (missing metadata:read,
  checks:read, organization_projects:write, actions_variables:read)
- Use forge.NewFakeClient() constructor (not struct literal)
- Fix LiveClient field name (http, not httpClient)
- Fix APIError field (Errors []APIErrorDetail, not Details)
- Fix TreeFile.Content type ([]byte, not string)
- Fix CommitFiles signature (no branch parameter)
- Fix mintcore dependency description (separate go.mod)
- Remove phantom DeleteRepoSecret from forge.Client table

Signed-off-by: Wayne Sun <gsun@redhat.com>
### CLI Wiring

```go
// internal/cli/admin.go (simplified)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] api-contract

The forge.Client method table omits 11 of 54 methods across several categories including PR file operations, workflow run details, and org secret/variable repo management.

Suggested fix: Add missing method categories to the table, or add an explicit note like 'Key categories shown; see internal/forge/forge.go for all ~54 methods' to set expectations.

@@ -0,0 +1,978 @@
# Go Developer Cookbook

A practical guide for Go developers contributing to the fullsend codebase. Covers project conventions, key patterns, and deep-dives into the mint, inference, GitHub forge, and reusable workflow systems.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] naming-convention

Introduction uses 'A practical guide for...' while all three other dev/ guides consistently use 'This guide [verb]...' pattern.

Suggested fix: Change to: 'This guide covers Go conventions, testing patterns, and deep-dives into the mint, inference, forge, and workflow systems for contributors to the fullsend codebase.'

Comment thread docs/guides/README.md
Guides for contributors developing and testing fullsend itself.

- [Local development](dev/local-dev.md) — Run fullsend agents locally on macOS and Linux (amd64 + arm64)
- [Go developer cookbook](dev/go-cookbook.md) — Go patterns, mint, inference, forge, and workflow internals for contributors

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] naming-convention

The cookbook entry description does not match the document introduction.

Suggested fix: Align the README entry description with the cookbook introduction.

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Jun 7, 2026
Cover the two remaining major subsystems:

- Sandbox system: container lifecycle, command execution, file transfer
  with symlink sanitization, provider management, runtime interface,
  bootstrap sequence, and harness configuration
- Security scanner: multi-phase pipeline architecture, scanner
  implementations (unicode, injection, redactor, SSRF, ONNX),
  Claude Code hooks, CLI commands, harness security config, and
  trace/audit system

Signed-off-by: Wayne Sun <gsun@redhat.com>
@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed ready-for-merge All reviewers approved — ready to merge labels Jun 7, 2026
@waynesun09 waynesun09 closed this Jun 7, 2026
@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #1995 — Go developer cookbook (docs-only, closed without merge)

Timeline: Human contributor (waynesun09) opened a docs-only PR adding a ~1500-line Go developer cookbook. The review bot's first pass was excellent — it caught genuinely incorrect code examples (wrong canonicalRolePermissions table across all 7 roles, phantom branch parameter in CommitFiles signature) at HIGH and MEDIUM severity. The author fixed these, the bot approved, then the author pushed additional content (sandbox + security scanner sections). The bot re-reviewed but found nothing new (0/8 findings actionable). CI then failed because detect-private-key pre-commit hook flagged a PEM key example in the docs. The PR was closed without merge.

Review quality assessment:

Signal-to-noise ratio degraded across runs: 40% → 0% → 0%.

Existing issue coverage: Several candidate proposals were filtered because they're already covered by open issues:

One novel proposal is included below.

Proposals filed

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

Labels

requires-manual-review Review requires human judgment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant