[fix] reject configuration setters after execution instead of silently applying - #30
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 6 |
| Duplication | -2 |
🟢 Coverage 100.00% diff coverage · +0.00% coverage variation
Metric Results Coverage variation ✅ +0.00% coverage variation (-1.00%) Diff coverage ✅ 100.00% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (b90ec7d) 1169 1160 99.23% Head commit (a8b7c67) 1175 (+6) 1166 (+6) 99.23% (+0.00%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#30) 58 58 100.00% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #30 +/- ##
=======================================
Coverage 98.96% 98.97%
=======================================
Files 12 12
Lines 871 876 +5
=======================================
+ Hits 862 867 +5
Misses 5 5
Partials 4 4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…y applying
Every config setter documented "It panics if called after execution", but the
guard was `if s.hasExec { log.DPanic(...) }` with no return, and the package
default logger is a no-op zap logger whose DPanic neither panics nor logs. So
under the default logger the guard was completely silent AND the change landed
anyway — the opposite of the documented contract, and a way to quietly mutate a
Box's security config (module set, step budget) after it has run.
A shared deniedAfterExec(action) helper now logs the misuse (still panicking
under a development logger, as the guard-triggering test relies on) and returns
true so each setter bails out WITHOUT applying the change — fail-closed under any
logger. The docs are corrected to "rejected: the change is ignored, and it
panics under a development logger".
Test: TestSetterAfterExecIsFailClosed — under the default logger a post-execution
SetMaxOutputEntries is ignored, so a later run is not constrained by a limit a
fail-open apply would have installed. The existing dev-logger panic test still
passes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
d53d516 to
a8b7c67
Compare
Problem
Every configuration setter's godoc promised "It panics if called after execution." The implementation was:
Two defects compound:
zap.NewNop().Sugar(), whoseDPanicneither panics nor logs. So with the default logger the guard was a complete no-op and the doc's contract was simply false.Fix (fail-closed)
deniedAfterExec(action): logs the misuse through the package logger (which still panics under a development logger, preserving that behaviour) and returnstrueso the caller bails out without applying the change.1:1fromif s.hasExec { log.DPanic(...) }toif s.deniedAfterExec("...") { return }(the 3 non-void settersEnableConsoleCapture/CreateMemory/AddHTTPContextreturn nil). No guard was missed; each still runs unders.mu.Test
TestSetterAfterExecIsFailClosed(test-first): with the default Nop logger, callingSetMaxOutputEntriesafter a run is silently ignored (no panic, no error) and a subsequent multi-entry run is unaffected — proving the mutation did not land. The existing development-logger panic table test continues to assert a panic per setter.Verification
gofmt/go vetclean;go test,go test -race -count=2, anddocker golang:1.19 go test -raceall green.