[aaydin-tr]

fix: /metrics index label per Backend, generic Middleware 500 body, raw Access log path

[github-actions]

Claude finished @aaydin-tr's task in 3m 24s —— View job

---

Code review of PR #51 (fixed point: origin/main)

I chose origin/main as the fixed point because the run is non-interactive. As instructed, the Spec axis is replaced by a Correctness axis. The Correctness sub-agent could not run go build, go vet or go test -race because the sandbox refused them, so its findings come from reading the code and tracing callers. Nothing was verified by a test run. Items already listed in TODO.md were excluded, such as the HTTP/2 open_conn_count stub.

Standards

Hard violation

• internal/proxy/proxy.go:353-357: the middlewareError doc comment is five lines. CLAUDE.md and CONTRIBUTING.md ("Code style") cap comments at one or two lines, and this PR added the last two. An inline comment has a suggested trim.

Judgement calls

• docs/middleware.md (about lines 141-173, 256, 271) calls OnRequest/OnResponse a "hook" ("What a hook returns", "no later hook runs"). CONTEXT.md lists "hook" under Middleware's Avoid list. It has no term for the two methods, so either add one or reword.
• internal/monitoring/prometheus.go:15,92,110-128: possible Data Clump. The address and index pair is declared as the backendLabels slice and the backendSeriesLabels struct, then spelled out as labels.address, labels.index in four WithLabelValues calls and one DeleteLabelValues call. The slice and the struct must stay in the same order by hand. A values() method on the struct would remove the repetition.
• internal/proxy/proxy.go:110 and :380: string(ctx.URI().PathOriginal()) appears at both Access log sites. Line 380 also uses an allocating string(...) where the neighbouring Method: line uses helper.B2S.
• The index is a string inside backendSeriesLabels. This is mild Primitive Obsession and acceptable, since Prometheus label values are strings.

Checked and clean

• Relative links in docs/, README.md, CONTRIBUTING.md and examples/README.md all resolve after the docs/ to development/ move.
• No stale docs/adr or docs/agents references remain in Go code or workflows.
• docs/ contains no ADR links, test names or CI details, and no CONTEXT.md avoid-synonyms.
• Names in the new tests and helpers are explanatory.
• The working-tree CLAUDE.md is modified and still points at docs/adr and docs/agents. That file is not part of this review run's changes. Don't commit it as it stands.

Correctness

Bugs

• None found. backendSeriesLabels is a comparable struct, so it works as a map key. A Reload-added Backend gets a never-used Index, so its series can't collide with a removed Backend's. Backends that share an address now get separate series instead of overwriting each other.

Security

• No new exposure.
  • The 500 body is now the constant {"message":"internal server error"}, so panic text and stack no longer reach the client.
  • The raw Access log path is escaped by zap's JSON encoder, so it can't forge a log line. Its size is bounded by server.read_buffer_size.
  • middlewareError now logs at Error level. A client that can trigger a Middleware error produces one Error line per request. That is a log-volume point, not a vulnerability.

Conflicts

• development/adr/0005-middleware-short-circuit-sentinel.md:3 still says divisor answers 500 {"message": …}. The code now sends the fixed body (internal/proxy/proxy.go:75), and docs/migration-1.0.md:259 documents the new body, so the ADR and the docs disagree. CONTRIBUTING.md says to amend an ADR with a new one, so add a short follow-up ADR or a one-line note. This line is not in the diff (the file is only renamed), so I couldn't attach it as an inline comment.
• Consumers of the old /metrics label and the old 500 body (integration test, unit tests, docs) are already updated.
• yaml:"-" on HealthCheckerFunc and HashFunc leaves Reload behaviour unchanged, because the classifier already skipped fields without a yaml name. ParseConfig now rejects those keys, and a test covers it.
• The sub-agent spot-checked docs against code and found them consistent: defaults, the Reload column, Access log fields, backend_hash and the index label, the 35-byte 503 body, the systemd unit, the compose file and the Alloy config. It did not check the "Since" version values in the config reference, the deployment guides, or docs/getting-started.md.

Summary: Standards has 1 hard violation and 4 judgement calls; the worst is the five-line middlewareError comment. Correctness has 1 finding, the stale ADR 0005 body text, and no bugs or security issues.
· branch feature/tier5-docs

[github-actions]

Claude finished @aaydin-tr's task in 3m 53s —— View job

---

Code review: PR #51 (origin/main...HEAD)

Choices made (non-interactive run): the fixed point is origin/main. The skill's spec lookup was skipped and the Spec axis replaced by a Correctness axis, as instructed. Both sub-agents ran in the foreground. Items already in TODO.md were excluded. The open_conn_count HTTP/2 stub is one of them and is already tracked there.

Standards

Sources: CLAUDE.md (Code style), CONTRIBUTING.md, CONTEXT.md, plus the Fowler smell baseline (judgement calls only).

Hard violations

1. internal/proxy/proxy.go:353-357: the middlewareError comment is five lines. CLAUDE.md and CONTRIBUTING.md cap comments at one or two. An inline suggestion is posted on the line.
2. development/adr/0005-middleware-short-circuit-sentinel.md:3: it still says a failed Middleware "answers 500 {"message": …}". This PR makes the body a fixed internal server error. CONTRIBUTING.md says to amend an ADR by writing a new one, and no ADR was added. The file is only renamed in this diff, so I couldn't attach an inline comment.
3. Commit dd98316: the subject docs: … fix: … joins two types. CONTRIBUTING.md "Pull requests" wants type(scope): summary and a focused change. This commit mixes about 3,000 lines of docs with three behaviour changes: the index metric label, the generic Middleware 500 body and the raw Access log path.

Smell judgement calls

• Data Clumps / Feature Envy, integration-test/monitoring_test.go:100-120: s *Scenario and backend *Echo travel together through backendSeries, bootIndex and requireBackendGauge. bootIndex only walks s.Backends, so it fits better as a Scenario method. It also returns -1 silently when the Backend isn't found.
• Duplicated Code, internal/monitoring/prometheus.go (about 108-114): WithLabelValues(labels.address, labels.index) is repeated four times, plus once in DeleteLabelValues. A small values() method on backendSeriesLabels would remove that.
• Duplicated Code, internal/proxy/proxy.go:111 and :381: string(ctx.URI().PathOriginal()) appears twice. NoAliveBackends also moved from helper.B2S to string(...) with no stated reason.
• Middle Man, internal/proxy/streaming_test.go:104 and backend_wire_test.go:99: newFasthttpFrontend and newHTTP2FrontendWithWriteTimeout now only wrap the …Serving variants. newFasthttpFrontend still has two callers, at streaming_test.go:601 and :634.
• Mysterious Name, streaming_test.go:136: newStreamingHTTP2Frontend no longer takes a ProxyClient, so "Streaming" is a misleading qualifier.
• Duplicated Code, pkg/logger/access_docs_test.go, pkg/config/config_reference_test.go, pkg/middleware/examples_test.go: each re-implements read-doc, normalize-CRLF and regex-table-parse. This is low priority because the packages differ.

Everything else conforms. New names use CONTEXT.md vocabulary, the other new comments are two lines or fewer, CONTEXT.md gained Application log and Monitoring server, and docs/ has no ADR or test-name leaks.

Correctness

Caveat: the sandbox denied every Go command. go build, go vet and go test -race were not run, so this is from reading the code only.

Bugs: none found.

• The new (address, index) series keys in prometheus.go:105-130 are consistent across WithLabelValues, DeleteLabelValues and the backendSeriesLabels map.
• BackendHash is the Backend's Index, which doesn't change on a Reload. A kept Backend keeps its series and a removed one is deleted. A changed entry gets a new Index and its old series is deleted in the same update, so cardinality stays bounded.
• Nothing else in the repo uses the old one-label form.
• The yaml:"-" tags on HealthCheckerFunc and HashFunc (config.go:205-206) break nothing. PlanReload already skipped - names, and both fields are set after parsing.

Security: no exploitable issue found.

• The Access log path now comes from PathOriginal(), so it is raw and attacker-controlled (proxy.go:110, :380). It is copied before preReq or any Middleware rewrite, and zap's JSON encoder escapes control characters, quotes and invalid UTF-8, so there is no log injection.
• middlewareError now returns a fixed generic body and keeps the error text in the Application log only. That removes a leak of Middleware internals to clients.

Conflicts

• The ADR 0005 mismatch above is the only code-versus-record conflict.
• Minor docs nit: docs/observability.md:77 says the path is logged "exactly as the client sent it". Invalid UTF-8 bytes come out as U+FFFD in the JSON line.
• The docs and examples checked match the code: defaults and validation in docs/configuration.md, Access log fields, /metrics labels, the systemd unit, the Kubernetes/Helm labels and preStop, the observability compose/Alloy/Prometheus files, and the .gitignore and .gitattributes changes. The review.md change is line-ending renormalisation only.

Summary

Standards: 3 hard violations and 6 judgement calls. The worst is the ADR 0005 text contradicting the new Middleware 500 body. Correctness: 0 bugs, 0 security findings and 2 conflicts. The worst is the same ADR mismatch.
· branch feature/tier5-docs