Skip to content

Commit 129b033

Browse files
josealekhinejosealekhine
committed
feat: add TestNoVariableShadowing, rename 70 bare err sites
Catches functions with multiple err := assignments. Convention requires descriptive names (readErr, writeErr, parseErr) so each error site is independently identifiable. General shadowing is handled by golangci-lint's shadow checker. Renamed 70 bare err := across 12 files in journal/parser/copilot*, cli/setup/core/copilot_cli/, cli/trace/core/, and cli/trace/cmd/. Signed-off-by: Jose Alekhinne <jose@ctx.ist>
1 parent ffd9388 commit 129b033

13 files changed

Lines changed: 259 additions & 86 deletions

File tree

‎internal/audit/variable_shadowing_test.go‎

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
// / ctx: https://ctx.ist
2+
// ,'`./ do you remember?
3+
// `.,'\
4+
// \ Copyright 2026-present Context contributors.
5+
// SPDX-License-Identifier: Apache-2.0
6+
7+
package audit
8+
9+
import (
10+
"go/ast"
11+
"go/token"
12+
"testing"
13+
)
14+
15+
// TestNoVariableShadowing detects two forms of variable
16+
// shadowing in non-test Go files:
17+
//
18+
// (a) Bare "err" reuse: multiple := assignments to
19+
// the unadorned name "err" in the same function body.
20+
// The convention requires descriptive names (readErr,
21+
// writeErr, parseErr) so each error site is
22+
// independently identifiable.
23+
//
24+
// (b) General inner-scope shadowing (any variable):
25+
// already caught by golangci-lint's shadow checker,
26+
// which is enabled in .golangci.yml.
27+
//
28+
// Test files are exempt.
29+
func TestNoVariableShadowing(t *testing.T) {
30+
pkgs := loadPackages(t)
31+
var violations []string
32+
33+
for _, pkg := range pkgs {
34+
for _, file := range pkg.Syntax {
35+
fpath := pkg.Fset.Position(file.Pos()).Filename
36+
if isTestFile(fpath) {
37+
continue
38+
}
39+
40+
for _, decl := range file.Decls {
41+
fn, ok := decl.(*ast.FuncDecl)
42+
if !ok || fn.Body == nil {
43+
continue
44+
}
45+
46+
sites := collectBareErrDefines(
47+
pkg.Fset, fn.Body,
48+
)
49+
50+
if len(sites) > 1 {
51+
for _, pos := range sites {
52+
violations = append(
53+
violations,
54+
pos+
55+
": bare err := in "+
56+
fn.Name.Name+
57+
" (use descriptive "+
58+
"name)",
59+
)
60+
}
61+
}
62+
}
63+
}
64+
}
65+
66+
if len(violations) > 0 {
67+
t.Errorf(
68+
"%d bare err := reuses found:",
69+
len(violations),
70+
)
71+
}
72+
limit := 30
73+
if len(violations) < limit {
74+
limit = len(violations)
75+
}
76+
for _, v := range violations[:limit] {
77+
t.Error(v)
78+
}
79+
if len(violations) > 30 {
80+
t.Errorf(
81+
"... and %d more",
82+
len(violations)-30,
83+
)
84+
}
85+
}
86+
87+
// collectBareErrDefines walks a function body and
88+
// returns positions of := assignments where "err" is
89+
// on the LHS.
90+
func collectBareErrDefines(
91+
fset *token.FileSet, body *ast.BlockStmt,
92+
) []string {
93+
var sites []string
94+
95+
ast.Inspect(body, func(n ast.Node) bool {
96+
assign, ok := n.(*ast.AssignStmt)
97+
if !ok {
98+
return true
99+
}
100+
101+
if assign.Tok != token.DEFINE {
102+
return true
103+
}
104+
105+
for _, lhs := range assign.Lhs {
106+
ident, ok := lhs.(*ast.Ident)
107+
if !ok {
108+
continue
109+
}
110+
111+
if ident.Name == "err" {
112+
sites = append(sites,
113+
posString(fset, assign.Pos()),
114+
)
115+
break
116+
}
117+
}
118+
119+
return true
120+
})
121+
122+
return sites
123+
}

‎internal/cli/setup/core/copilot_cli/agent.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,13 +31,13 @@ func deployAgent(cmd *cobra.Command) error {
3131
agentsDir := filepath.Join(cfgHook.DirGitHub, cfgHook.DirGitHubAgents)
3232
target := filepath.Join(agentsDir, cfgHook.FileAgentsCtxMd)
3333

34-
if _, err := os.Stat(target); err == nil {
34+
if _, statErr := os.Stat(target); statErr == nil {
3535
writeSetup.InfoCopilotCLISkipped(cmd, target)
3636
return nil
3737
}
3838

39-
if err := ctxIo.SafeMkdirAll(agentsDir, fs.PermExec); err != nil {
40-
return err
39+
if mkErr := ctxIo.SafeMkdirAll(agentsDir, fs.PermExec); mkErr != nil {
40+
return mkErr
4141
}
4242

4343
content, readErr := agent.AgentsCtxMd()

‎internal/cli/setup/core/copilot_cli/copilot_cli.go‎

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -41,14 +41,14 @@ func Deploy(cmd *cobra.Command) error {
4141
targetJSON := filepath.Join(hooksDir, cfgHook.FileCopilotCLIHooksJSON)
4242

4343
// Check if ctx-hooks.json already exists
44-
if _, err := os.Stat(targetJSON); err == nil {
44+
if _, statErr := os.Stat(targetJSON); statErr == nil {
4545
writeSetup.InfoCopilotCLISkipped(cmd, targetJSON)
4646
return nil
4747
}
4848

4949
// Create directories
50-
if err := ctxIo.SafeMkdirAll(scriptsDir, fs.PermExec); err != nil {
51-
return errFs.Mkdir(scriptsDir, err)
50+
if mkErr := ctxIo.SafeMkdirAll(scriptsDir, fs.PermExec); mkErr != nil {
51+
return errFs.Mkdir(scriptsDir, mkErr)
5252
}
5353

5454
// Write ctx-hooks.json
@@ -76,23 +76,27 @@ func Deploy(cmd *cobra.Command) error {
7676
}
7777

7878
// Write .github/agents/ctx.md
79-
if err := deployAgent(cmd); err != nil {
80-
writeErr.WarnFile(cmd, cfgHook.DirGitHubAgents, err)
79+
if agentErr := deployAgent(cmd); agentErr != nil {
80+
writeErr.WarnFile(cmd, cfgHook.DirGitHubAgents, agentErr)
8181
}
8282

8383
// Write .github/instructions/context.instructions.md
84-
if err := deployInstructions(cmd); err != nil {
85-
writeErr.WarnFile(cmd, cfgHook.DirGitHubInstructions, err)
84+
if instrErr := deployInstructions(cmd); instrErr != nil {
85+
writeErr.WarnFile(
86+
cmd, cfgHook.DirGitHubInstructions, instrErr,
87+
)
8688
}
8789

8890
// Register ctx MCP server in ~/.copilot/mcp-config.json
89-
if err := ensureMCPConfig(cmd); err != nil {
90-
writeErr.WarnFile(cmd, cfgHook.FileMCPConfigJSON, err)
91+
if mcpErr := ensureMCPConfig(cmd); mcpErr != nil {
92+
writeErr.WarnFile(
93+
cmd, cfgHook.FileMCPConfigJSON, mcpErr,
94+
)
9195
}
9296

9397
// Write .github/skills/<name>/SKILL.md for Copilot CLI skills
94-
if err := deploySkills(cmd); err != nil {
95-
writeErr.WarnFile(cmd, cfgHook.DirGitHubSkills, err)
98+
if skillErr := deploySkills(cmd); skillErr != nil {
99+
writeErr.WarnFile(cmd, cfgHook.DirGitHubSkills, skillErr)
96100
}
97101

98102
writeSetup.InfoCopilotCLISummary(cmd)

‎internal/cli/setup/core/copilot_cli/instructions.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,13 +32,13 @@ func deployInstructions(cmd *cobra.Command) error {
3232
instrDir := filepath.Join(cfgHook.DirGitHub, cfgHook.DirGitHubInstructions)
3333
target := filepath.Join(instrDir, cfgHook.FileInstructionsCtxMd)
3434

35-
if _, err := os.Stat(target); err == nil {
35+
if _, statErr := os.Stat(target); statErr == nil {
3636
writeSetup.InfoCopilotCLISkipped(cmd, target)
3737
return nil
3838
}
3939

40-
if err := ctxIo.SafeMkdirAll(instrDir, fs.PermExec); err != nil {
41-
return err
40+
if mkErr := ctxIo.SafeMkdirAll(instrDir, fs.PermExec); mkErr != nil {
41+
return mkErr
4242
}
4343

4444
content, readErr := agent.InstructionsCtxMd()

‎internal/cli/setup/core/copilot_cli/skills.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,13 +38,13 @@ func deploySkills(cmd *cobra.Command) error {
3838
skillDir := filepath.Join(skillsBase, name)
3939
target := filepath.Join(skillDir, cfgHook.FileSKILLMd)
4040

41-
if _, err := os.Stat(target); err == nil {
41+
if _, statErr := os.Stat(target); statErr == nil {
4242
writeSetup.InfoCopilotCLISkipped(cmd, target)
4343
continue
4444
}
4545

46-
if err := ctxIo.SafeMkdirAll(skillDir, fs.PermExec); err != nil {
47-
return err
46+
if mkErr := ctxIo.SafeMkdirAll(skillDir, fs.PermExec); mkErr != nil {
47+
return mkErr
4848
}
4949
if wErr := ctxIo.SafeWriteFile(target, content, fs.PermFile); wErr != nil {
5050
return wErr

‎internal/cli/trace/cmd/tag/run.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,9 @@ func Run(cmd *cobra.Command, commitRef, note string) error {
3636
return errTrace.NoteRequired()
3737
}
3838

39-
hash, err := trace.ResolveCommitHash(commitRef)
40-
if err != nil {
41-
return errTrace.ResolveCommit(commitRef, err)
39+
hash, resolveErr := trace.ResolveCommitHash(commitRef)
40+
if resolveErr != nil {
41+
return errTrace.ResolveCommit(commitRef, resolveErr)
4242
}
4343

4444
traceDir := filepath.Join(rc.ContextDir(), dir.Trace)
@@ -48,8 +48,8 @@ func Run(cmd *cobra.Command, commitRef, note string) error {
4848
Refs: []string{fmt.Sprintf("%q", note)},
4949
}
5050

51-
if err := trace.WriteOverride(entry, traceDir); err != nil {
52-
return errTrace.WriteOverride(err)
51+
if writeErr := trace.WriteOverride(entry, traceDir); writeErr != nil {
52+
return errTrace.WriteOverride(writeErr)
5353
}
5454

5555
writeTrace.Tagged(cmd, trace.ShortHash(hash), note)

‎internal/cli/trace/core/collect/collect.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -47,9 +47,9 @@ func RecordCommit(commitHash string) error {
4747
return nil
4848
}
4949

50-
message, err := trace.CommitMessage(commitHash)
51-
if err != nil {
52-
return errTrace.GitLog(err)
50+
message, msgErr := trace.CommitMessage(commitHash)
51+
if msgErr != nil {
52+
return errTrace.GitLog(msgErr)
5353
}
5454

5555
traceDir := filepath.Join(contextDir, dir.Trace)
@@ -58,8 +58,8 @@ func RecordCommit(commitHash string) error {
5858
Refs: refs,
5959
Message: message,
6060
}
61-
if err := trace.WriteHistory(entry, traceDir); err != nil {
62-
return errTrace.WriteHistory(err)
61+
if histErr := trace.WriteHistory(entry, traceDir); histErr != nil {
62+
return errTrace.WriteHistory(histErr)
6363
}
6464

6565
stateDir := filepath.Join(contextDir, dir.State)

‎internal/cli/trace/core/hook/hook.go‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -73,15 +73,15 @@ func Enable(cmd *cobra.Command) error {
7373
// Returns:
7474
// - error: non-nil on removal failure
7575
func Disable(cmd *cobra.Command) error {
76-
prepPath, err := FilePath(cfgGit.HookPrepareCommitMsg)
77-
if err != nil {
78-
return err
76+
prepPath, prepErr := FilePath(cfgGit.HookPrepareCommitMsg)
77+
if prepErr != nil {
78+
return prepErr
7979
}
8080
Remove(prepPath)
8181

82-
postPath, err := FilePath(cfgGit.HookPostCommit)
83-
if err != nil {
84-
return err
82+
postPath, postErr := FilePath(cfgGit.HookPostCommit)
83+
if postErr != nil {
84+
return postErr
8585
}
8686
Remove(postPath)
8787

@@ -99,18 +99,18 @@ func Disable(cmd *cobra.Command) error {
9999
// Returns:
100100
// - error: non-nil if a non-ctx hook already exists or write fails
101101
func Install(path, script, name string) error {
102-
if _, err := io.SafeStat(path); err == nil {
102+
if _, statErr := io.SafeStat(path); statErr == nil {
103103
existing, readErr := io.SafeReadUserFile(path)
104104
if readErr == nil && !strings.Contains(
105105
string(existing), cfgTrace.CtxTraceMarker,
106106
) {
107107
return errTrace.HookExists(name, path)
108108
}
109109
}
110-
if err := io.SafeWriteFile(
110+
if writeErr := io.SafeWriteFile(
111111
path, []byte(script), cfgFs.PermExec,
112-
); err != nil {
113-
return errTrace.HookWrite(name, err)
112+
); writeErr != nil {
113+
return errTrace.HookWrite(name, writeErr)
114114
}
115115
return nil
116116
}

0 commit comments

Comments
 (0)