Code review: robustness, security, and tooling findings

[alexander-yevsyukov]

Code review: robustness, security, and tooling findings

A pass over the whole codebase (main.go, cli, embedding, embedding/parsing,
fragmentation, files, analyzing, configuration, indent). Tests pass and gofmt/go vet
are clean. The items below are grouped by impact, with file/line references. Where a finding
overlaps an existing issue it is noted rather than restated.

A. Robustness — panic is used for expected/operational errors

There are ~36 panic calls in non-test code, and the CLI's actual work panics rather than
returning errors:

• cli.CheckCodeSamples / EmbedCodeSamples / AnalyzeCodeSamples (cli/cli.go:95-124) panic on
  any failure. main.go carefully handles validation errors and logs them via slog, but the
  operations themselves crash the process with a Go stack trace instead of a readable message.
• Instruction.matchGlob (embedding/parsing/instruction.go:165) panics with
  there is no line matching ... when a user-supplied start/end pattern doesn't match — a
  normal user-input mistake surfaced as a crash.

Recommendation: thread errors up to main and print a single clean diagnostic + non-zero exit
code. Complements #23 (Improve error feedback).

B. Bug — UnexpectedDiffError is thrown by value but Error() has a pointer receiver

errors.go:33 defines func (e *UnexpectedDiffError) Error() string, but
embedding/processor.go:145 does panic(UnexpectedDiffError{changedFiles}) — a value, which
does not implement the error interface (only *UnexpectedDiffError does). The panic therefore
prints the raw struct rather than the intended unexpected diff: ... message, and any
recover() that type-asserts to error would miss it. Fix: panic with &UnexpectedDiffError{...}
or switch to a value receiver.

C. Security/quality — documentation files are written world-writable and executable (0777)

• embedding/processor.go:84 writes the target doc files with files.ReadWriteExecPermission
  (0777, defined at files/files.go:34). Markdown/HTML are data, not executables, and 0777 is
  world-writable.
• files.EnsureDirExists (files/files.go:104) also creates directories with 0777.
• This is inconsistent with fragment files, which already use 0600 (fragmentation/fragment_file.go:79).

The gosec linter (enabled in .golangci.yml) flags exactly this. Recommendation: 0644 for the
written docs, 0755 only where a directory truly needs it.

D. Bug — files.ReadFile silently truncates on a read error

files/files.go:75-83:

line, _, err := reader.ReadLine()
if err != nil {
    break
}

Any error — not just io.EOF — ends the loop, and the function then returns a nil error,
silently dropping the remainder of the file. Distinguish EOF from real errors and propagate the
latter.

E. Tooling — .golangci.yml exists but is never enforced, and one file already fails it

• There are no .github/workflows in the repo (checked on this branch and master), so the
  comprehensive lint config in .golangci.yml never runs. (Add basic GH workflows #11 Add basic GH workflows is closed,
  but no workflow files are present.)
• Concretely: analyzing/analyzing.go is missing the copyright header that every other .go
  file carries, which the enabled goheader linter would fail on.

Recommendation: add a CI workflow that runs go test ./... + golangci-lint run, and add the
missing header.

F. Repository hygiene

• No .gitignore. IDE artifacts (.idea/, *.iml) and build/ outputs are trivially
  committed by accident — the working tree already shows an untracked .idea/, and committed
  *.iml files exist under embed-code-go/test/resources/.
• ~12 MB of pre-compiled binaries committed under bin/ (embed-code-macos,
  embed-code-ubuntu, embed-code-win.exe). Binaries in VCS bloat history and go stale; prefer
  GitHub Releases or go install.
• A second, nearly-empty nested embed-code-go/ directory (duplicating the module name) holds only
  .iml files — confusing layout.
• No LICENSE file, despite every source file carrying a TeamDev copyright + disclaimer header.

G. Minor / docs

• -mode flag help (cli/cli.go:144) says the mode "can be 'check' or 'embed'", omitting
  analyze; parts of the main.go doc comment do the same.
• removeElements (embedding/processor.go:249) documents "Removes elements of the second list
  from the first one," but actually returns included minus excluded (second-minus-first). The
  comment and the parameter names (first=excluded, second=included) are misleading.
• Processor.FindChangedEmbeddings (embedding/processor.go:101-107) calls
  context.FindChangedEmbeddings() before checking err, and the two error paths in
  fillEmbeddingContext are inconsistent: the moveToNextState path returns an empty Context{}
  while the !accepted path returns a populated context. Tighten the error/value contract.
• Test coverage gaps: analyzing, configuration, and the root main package have no tests
  (go test ./... reports "no test files"). Complements Add integration test for the embed-code #24 (Add integration test).

[alexander-yevsyukov]

@Oleg-Melnik, this is a result of the code review by Claude. Please look into this and work with your colleagues on addressing these findings. I believe most of the changes can be vibe-coded.

[alexander-yevsyukov]

panic is used for expected/operational errors

This is the main concern from me as the user of the utility. I've experience those "panic" things a lot.