Skip to content

Add a code audit findings document - #506

Draft
danmux wants to merge 1 commit into
mainfrom
dm-claude/code-audit
Draft

Add a code audit findings document#506
danmux wants to merge 1 commit into
mainfrom
dm-claude/code-audit

Conversation

@danmux

@danmux danmux commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Summary

A point-in-time review of the repo for correctness, duplication and adherence to our own
conventions in AGENTS.md and ARCHITECTURE.md. Docs only, no code touched.

62 findings in docs/CODE_AUDIT.md, grouped and ordered by severity, each with the defect,
why it matters and the fix. Audited at 6bb97f9; every file:line was re-derived against
that commit.

The ones worth reading first

  • internal/github stacks its own retry on top of httpcl's, so a GitHub 5xx costs up to
    12 HTTP attempts and ~27s per GraphQL query. One field fixes it.
  • main.go reads userError through type assertions, not errors.As, so wrapping one
    discards message, detail, suggestion, code and exit code. Confirmed by running it, not just
    reading it.
  • gitutil runs git against the process cwd with no dir or ctx, so
    --project /elsewhere diffs the wrong repo on the --checkout sync path. GeneratePatch
    also mutates the user's index with a swallowed cleanup error.
  • Stack detection exists twice and the copies disagree: chunk init writes pytest where
    the sidecar builds uv run pytest. Most likely of these to get reported as a bug by someone
    who has no idea two detectors exist.
  • The auth provider matrix is hand-written 18 times across 3 files. internal/cmd/auth.go
    is 727 lines and almost all of it is this.
  • envbuilder has 31 stack-conditional branches across 3 parallel switches, which is the
    root cause of both nolint:gocyclo. The doc proposes splitting by stack rather than by phase.

Also in there

  • What was checked and found clean, so none of it gets re-checked. Layering holds, output
    discipline holds, all 12 test skips are justified, gitutil is not over-exported.
  • Why several of these got through: dupl is not enabled, gocyclo has no min-complexity,
    and funlen is excluded for tests but never enabled at all.
  • A suggested order of work, cheapest and highest value first.

Notes for review

  • Deliberately not added to the documentation map in AGENTS.md. It is a point-in-time
    report that will go stale, not a standing reference like ARCHITECTURE.md or CLI.md.
  • One finding (validate --list swallowing a config load error) is already addressed by preserve unknown config keys in ‘chunk config set’ #497
    and is marked as such so it does not get fixed twice.
  • Findings marked [swept] came from a broad automated sweep and were not individually
    re-verified by hand. Everything else was confirmed by reading the code.

🤖 Generated with Claude Code

A point-in-time review of the repo for correctness, duplication and
adherence to our own conventions. Docs only, no code touched.

62 findings, grouped and ordered by severity, each with the defect,
why it matters and the fix. Highlights:

- internal/github stacks its own retry on top of httpcl's, so a 5xx
  costs up to 12 HTTP attempts and ~27s per GraphQL query
- main.go reads userError through type assertions rather than
  errors.As, so wrapping one discards the whole payload
- gitutil runs git against the process cwd with no dir or ctx, so
  --project elsewhere diffs the wrong repo
- stack detection exists twice and the copies disagree: init writes
  pytest where the sidecar builds uv run pytest
- the auth provider matrix is hand-written 18 times across 3 files
- envbuilder has 31 stack branches across 3 parallel switches

Also records what was checked and found clean, so none of it gets
re-checked, and notes where dupl, gocyclo and funlen configuration
let several of these through.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant