Skip to content

chore: add AGENTS.md and PLAN.md#8

Merged
polarn merged 2 commits into
mainfrom
chore/add-agents-and-plan-md
Jul 2, 2026
Merged

chore: add AGENTS.md and PLAN.md#8
polarn merged 2 commits into
mainfrom
chore/add-agents-and-plan-md

Conversation

@polarn

@polarn polarn commented Jul 2, 2026

Copy link
Copy Markdown
Owner

Summary

  • AGENTS.md — Agent-facing operational document: build commands, directory structure, Go version, architecture, and known critical issues to avoid regressions.
  • PLAN.md — Comprehensive findings from code analysis covering bugs, security issues, missing validation, code quality, design improvements, and portability — with recommended priority order.

@polarn

polarn commented Jul 2, 2026

Copy link
Copy Markdown
Owner Author

Review: fact-check of AGENTS.md / PLAN.md against the code

Since these docs exist to steer future agents/maintainers, I verified every factual claim against the code on main. Much of it checks out (provider order, missing HTTP/context timeouts, hardcoded GitLab URL, duplicate has*() helpers, --dry-run printing secrets), but eight claims are wrong — and several would cause actively harmful "fixes" if followed as written.

Confirmed inaccuracies (most-severe first)

  1. AGENTS.md:29 — the interface method is Provide, not Fetch. internal/provider/provider.go:8 declares Provide(config *config.RootConfig, envVars map[string]string) error (the doc also omits the error return). No Fetch exists anywhere. An agent adding a provider per the doc writes a method that never satisfies the interface.

  2. PLAN.md Security §1 + AGENTS.md:38 — the "shell injection" vulnerability doesn't exist. internal/env/env.go:12-13 wraps values in single quotes and escapes embedded ' with the standard POSIX-safe '\'' idiom. Inside single quotes, $, `, and \ are literal — not injection vectors. The suggested fix (escaping those characters) would corrupt correct output with spurious backslashes. Note this non-bug is also half of PLAN.md's Priority-1 row.

  3. PLAN.md Bug §8 — the value/valueFrom precedence is stated backwards. Providers run plain → gcp → gitlab (provider.go:28-32); plain.go:9 writes value first, then gcp.go:53 / gitlab.go:50 overwrite the same map key. So valueFrom wins, not value. Following the suggested fix ("document that value always overrides valueFrom") would enshrine the opposite of runtime behavior. Root cause: the warning text at validation.go:40 ("value takes precedence") is itself wrong — that's the actual bug worth fixing.

  4. AGENTS.md:31 (and :11) — there is no upward directory search for config. internal/config/loader.go:11-16 reads .env-exec.yaml from the current working directory only (or the literal ENV_EXEC_YAML path), and silently returns nil when it's missing. No walk up the tree.

  5. AGENTS.md:31 — there is no --config flag. cmd/env-exec/main.go:46-59 parses only --help/-h, --version/-v, --dry-run/-n; anything else is treated as the command to exec, so env-exec --config foo.yaml cmd tries to execute a binary literally named --config.

  6. PLAN.md Bug §1 / Portability §1 + AGENTS.md:37 — syscall.WaitStatus is not Unix-only. On Windows, syscall.WaitStatus exists and ProcessState.Sys() returns it, so the assertion at exec.go:21 succeeds and exit codes propagate fine. It's also not "silently lost": on assertion failure the code falls through to exec.go:25 and main.go:86 calls log.Fatalf. Switching to exitError.ExitCode() is still a nice simplification, but the "broken on Windows" premise (Priority 1 in PLAN.md, "Do NOT Regress" in AGENTS.md) is false.

  7. PLAN.md Bug §4 + AGENTS.md:40 — wrong line range for the GCP silent fetch failure. gcp.go:37-41 is the missing-project skip; the actual fetch-failure warn-and-continue is gcp.go:48-51. Anyone implementing "make fetch failures fatal" at the cited lines patches the wrong block. (The gitlab cite 40-47 is correct.)

  8. PLAN.md Bug §6 — the suggested fix doesn't compile. defer os.Stdout = old is a syntax error (defer requires a call). Correct form: defer func() { os.Stdout = old }(). The underlying observation (no defer in captureStdout) is valid.

Minor notes

  • PLAN.md Security §3 — PRIVATE-TOKEN is not deprecated by GitLab; both it and Authorization: Bearer are documented and supported.
  • PLAN.md Security §6 — behavior with both refs set isn't "undefined": the order is fixed, so it's deterministic last-writer-wins (gitlab overwrites gcp).
  • PLAN.md Security §7 — the suggested fix is unrelated to the stated issue; resp.Body.Close() is already deferred at gitlab.go:81.
  • AGENTS.md:41 — "All providers use log.Printf": plain.go has no logging at all; only gcp and gitlab do.
  • Bonus code finding (out of this PR's scope): validation.go:44-47 is dead code — hasGCP is defined as GCPSecretKeyRef.Name != "", so the inner empty-name check can never fire.

Given #2 and #6, the Priority-1 row of PLAN.md's "Recommended Priority Order" is built on two non-bugs and probably deserves a re-rank.

🤖 Generated with Claude Code

- Fix provider interface method name: Fetch → Provide (with error return)
- Fix config loading description: no upward search, no --config flag
- Remove shell injection claim (single quotes protect against $/``/\)
- Remove Windows exit code claim (syscall.WaitStatus works on Windows)
- Fix GCP fetch failure line numbers (48-51, not 37-41)
- Fix: plain.go has no logging, only gcp and gitlab do
- Add new finding: validation.go:40 says 'value takes precedence' but valueFrom wins
- Add new finding: validation.go:44-47 is dead code (Name != '' ⇒ Name == '' never fires)
- Remove PRIVATE-TOKEN deprecation claim (still supported by GitLab)
- Remove Body.Close() claim (already deferred at gitlab.go:81)
- Remove 'undefined behavior' for dual valueFrom (deterministic last-writer-wins)
- Fix defer syntax (os.Stdout = old is not a call; use defer func() { ... }())
- Reweight priority order to remove non-bugs
@polarn polarn merged commit 3c44420 into main Jul 2, 2026
2 checks passed
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