Skip to content

Add Go error-handling and constant-usage conventions to go-code.md #6247

Description

@fullsend-ai-retro

What happened

On PR #6242, the code agent's initial implementation (commit b04d906) contained 4 convention violations that required 3 human fix iterations before the review agent approved:

  1. Used string literals "github" and "gitlab" instead of repos.ForgeGitHub/repos.ForgeGitLab typed constants (found Review 1)
  2. Used fmt.Errorf for token-missing errors instead of sentinel errors with errors.Is (found Review 3)
  3. Used strings.Contains(err.Error(), "--gitlab-token") for error detection instead of errors.Is (found Review 3)
  4. Used literal escaped quotes instead of %q in error messages, inconsistent with tracker_client.go (found Review 3)

The human (ggallen) introduced errGitLabTokenMissing as a sentinel error in forge_client.go, switched to errors.Is, adopted repos.ForgeGitLab/repos.ForgeGitHub constants, and used %q formatting — roughly doubling the PR's line count from the code agent's initial 105 additions to the final 229 additions.

What could go better

The docs/contributing/go-code.md guide covers testing, coverage, linting, concurrency patterns, interface documentation, and mint sync rules, but contains no guidance on error-handling patterns or constant/naming conventions. The code agent reads AGENTS.md which points to go-code.md for Go changes — but go-code.md is silent on the patterns that caused all 4 violations.

I am confident this documentation gap contributed to the rework: the code agent had no written rule to follow, and the conventions are not enforceable by the repo's linter configuration (.golangci.yml enables errcheck, govet, staticcheck, unused, gosimple, ineffassign — none of which flag string-literal-vs-constant or fmt.Errorf-vs-sentinel choices). Similarly, the forge-abstraction.md guide mandates using forge.Client but does not mention using typed forge name constants.

I am moderately confident that documenting these conventions would reduce this class of violation. The code agent explicitly reads the contributing guides and follows their rules (e.g., it ran tests, checked coverage, used the forge interface). Adding explicit error-handling and constant-usage rules gives it concrete guidance to follow. There is some uncertainty about whether the code agent will proactively search for existing constants even when told to — that is a deeper agent capability issue tracked in fullsend-ai/agents#288.

Proposed change

Add an "Error handling and naming conventions" section to docs/contributing/go-code.md after the existing "Concurrency testing" section. The section should cover:

  1. Use typed constants over string literals. When the codebase defines constants for a value (e.g., repos.ForgeGitHub, repos.ForgeGitLab), use them instead of repeating string literals. Before introducing a string literal for a domain value, search for existing constants: grep -rn 'const.*ForgeGitHub\|ForgeGitLab' internal/.

  2. Prefer sentinel errors for programmatic error checking. When callers need to distinguish error conditions, define a package-level sentinel (var errXxx = errors.New(...)) and check with errors.Is. Do not use strings.Contains(err.Error(), ...) — it couples error handling to message wording and breaks when messages change.

  3. Use %q for user-facing values in error messages. Format user-provided or enumerated values with %q (e.g., fmt.Errorf("unsupported forge %q", name)) for consistent quoting. This matches the pattern in tracker_client.go and other CLI commands.

  4. Consistent error message content. When multiple code paths produce "no token found" errors for different forges, ensure they all mention the same remediation options (environment variables AND --token flag) so users see consistent guidance regardless of which path triggers.

Validation criteria

  1. On the next 5 code agent PRs that modify Go code in internal/cli/ or internal/forge/, check whether the code agent uses existing typed constants and sentinel error patterns. The rate of review findings in the 'convention violation' category (string literals vs constants, fmt.Errorf vs sentinel, string matching vs errors.Is) should decrease compared to the baseline (4 violations on PR feat(#6240): add --forge and --base-url flags to post-review #6242).
  2. Spot-check: create a test issue requesting a new forge-switching function and verify the code agent uses repos.ForgeGitHub/repos.ForgeGitLab constants and sentinel errors in its initial implementation.
  3. The review agent should not need to flag constant-usage or error-pattern violations on PRs where the code agent read the updated go-code.md.

Generated by retro agent from #6242

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    Status
    In progress

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions