Correct release-blocking documentation inaccuracies (closes #141) #144
Reference in New Issue
Block a user
Delete Branch "issue-141-release-readiness"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #141.
What changed
README.mdenv table gainsRETENTION_SWEEP_INTERVAL(default1h), which two other passages already referred to as documented.TODO.mdCompleted Steps gains the five omitted units: #118, #64, #90, #113, #79. Derived fromgit log origin/main..origin/next, not from the existing text.TODO.mdFuture Steps regains "Manual event redelivery from the web UI". Redelivery itself is not implemented here; that is a feature and out of scope.console.log("Webhooker loaded")removed fromstatic/js/app.js.No behavioural change. The only non-comment, non-documentation edit is the deleted
console.log.Rework
Rebased onto
next(nowbe57609) and addressed the review finding.Finding fixed.
README.md:788-792, the delivery-semantics passage on target-type edits, said a terminally failed delivery's event "can be redelivered manually". It now says the event stays stored but there is no way to redeliver it, and that manual redelivery is planned, not implemented.Full README sweep. The previous pass fixed only the lines the issue named. Every occurrence of redelivery / replay / resend / re-send / re-dispatch / manual recovery in
README.md, in any tense (line numbers as ofc4022c0):POST /api/v1/events/{id}/redeliver#### API (Planned)heading. Accurate, unchanged.retry/max_retries/retrying/ retry sweep /Retry-Afterinternal/delivery/engine.go). Not redelivery. Accurate, unchanged.RecoverermiddlewareNothing outside the README implements or claims redelivery:
redeliver/replay/resend/re-sendacross all.go,.html,.jsand.tmplfiles (excluding vendoredstatic/js/alpine.min.js) matched exactly one line.Judgement call on
README.md:506-507. The reviewer flagged "Stores the complete HTTP request data for replay and auditing" as a decision to make rather than a defect. Call: changed it. It is a purpose statement rather than a present-tense feature promise, so the reviewer's reading is fair — but a bare "for replay" in the data-model section is the same residual claim that caused this rework, and the fix costs four words. Changing it makes the rule uniform across the file: after this commit, every mention of replay or redelivery in the README is marked planned or sits under a Planned heading, with no case left to argue about.One edit outside the README, disclosed. The doc comment on
failUnretryableRetry(internal/delivery/engine.go:788) was the source text for the README passage and carried the identical false claim — "failed with a recorded reason and can be redelivered manually". Since the issue is titled "docs contradict the code", leaving the same untrue sentence in the tree while fixing it in the README seemed worse than a three-line comment edit. It now reads "The event stays stored, but nothing redelivers it today." Comment only; no code, no behaviour.Env-table conflict resolved. #142 landed the same
RETENTION_SWEEP_INTERVALrow, so the rebase conflicted on it. Kept one row combining what each version got right, rather than either verbatim:next(from #142)Verified against the source, not assumed. Default
1h:defaultRetentionSweepInterval = time.Hour(internal/config/config.go:34) — both versions were right. Two consumers, not one:internal/database/retention.go:56andinternal/delivery/archive_sweeper.go:61both readConfig.RetentionSweepInterval, so this PR's description was the more complete of the two. Must be positive: true only as of #142, which moved the variable toenvPositiveDuration(internal/config/config.go:378), so that constraint had to be preserved fromnext. The default column is1heither way.Rebased a second time after #138 landed on
nextmid-rework; that one was a clean rebase with no conflict, and the whole gate was re-run on the result.Env-var enumeration
Every variable read by
internal/config, taken from the source rather than by eye (os.Getenvcall sites plus theenvString/envBool/envPort/envDuration/envPositiveDuration/envPositiveInt/envPrefixListhelpers), then diffed against the table. Twelve variables:WEBHOOKER_ENVIRONMENT,PORT,DATA_DIR,DEBUG,MAINTENANCE_MODE,METRICS_USERNAME,METRICS_PASSWORD,SENTRY_DSN,RETENTION_SWEEP_INTERVAL,SESSION_IDLE_TIMEOUT,RECEIVER_RATE_LIMIT,TRUSTED_PROXIES.RETENTION_SWEEP_INTERVALwas the only gap; the table now lists all twelve. No package outsideinternal/configreads the environment —Getenv/LookupEnvappear nowhere else in the tree.Debug-logging sweep:
console.*anddebuggeracrossstatic/andtemplates/matched only the one line inapp.js.static/js/alpine.min.jsis a vendored third-party bundle and was left alone.Gate
Re-run in full on the final rebased commit
c4022c0.make checkexit 0,0 issues.docker build --no-cache-filter=lint,builder --progress=plain .exit 0, image exported. Evidence the stages executed rather than replaying cache:#20 [lint 7/8] RUN make fmt-check DONE 0.2s,#21 [lint 8/8] RUN make lintprinting0 issues.at 62.95s andDONE 67.3s,#28 [builder 8/10] RUN make test DONE 73.4sand#29 [builder 9/10] RUN make build DONE 50.8s, with zero(cached)markers anywhere in the log. The onlyCACHEDlayers in the build are the two base-image resolves (#1,#3),stage-2steps (#8,#9,#10,#31,#32,#33) and the lintCOPY go.mod go.sum/go mod downloaddedup aliases (#17,#18); no lint or builderRUNlayer that does work was cached.Note, not fixed here and out of scope: the linter warns
The linter 'gomodguard' is deprecated (since v2.12.0) ... Replaced by gomodguard_v2.FAIL — needs-rework. One finding.
README.md:785-787— a third present-tense redelivery claim is left standing, and it now contradicts this commit's own text.This states, in the present tense, that an operator can manually redeliver — the exact claim #141 requires the README to stop making, and it directly contradicts
README.md:266, added by this commit: "No redelivery exists today, in the web UI or the API". It is also the worst place to leave it: the passage is about a delivery that was terminally failed by a target-type edit, so it tells the operator that a lost delivery is recoverable by hand. It is not — nothing in the tree implements redelivery under any name (confirmed:redeliver,replay,resendacross the whole tree yield only comments, docs and the planned-API row).The PR fixed only the two lines the issue named; the issue's definition of done is about the README, not those two lines. Acceptable: make the clause conditional on the planned feature ("...so it can be redelivered once manual redelivery lands (planned)"), or drop the trailing clause.
Judgement call, not required:
README.md:503"Stores the complete HTTP request data for replay and auditing" reads as a statement of what the stored data is for, consistent with the newREADME.md:264-265, so I am not calling it a defect — flagging it so it is a decision rather than an oversight.Everything else verified and correct: all twelve env vars enumerated independently from
internal/config/config.gomatch the table exactly, and every documented default matches the code (PORT8080,DATA_DIR/var/lib/webhookerviaconfig.go:415-416,RETENTION_SWEEP_INTERVAL1h,SESSION_IDLE_TIMEOUT24h,RECEIVER_RATE_LIMIT120, rest empty/false); noos.Getenv/LookupEnvoutsideinternal/config; the new row's description is accurate (the interval really does drive bothinternal/database/retention.go:56andinternal/delivery/archive_sweeper.go:61); all 14 issue-closing units ingit log origin/main..origin/nextappear in Completed Steps, and the five new entries were checked against the code rather than the issue titles, includingmaskURLError(#118) andTargetView/header-count (#113); theconsole.logdeletion is inert (nothing referenced it, no test asserts it) and no otherconsole.*/debuggerremains instatic/ortemplates/; single commit, no attribution trailers, title ends(closes #141), basenext, fast-forward onto currentnext, added markdown wraps at the file's 72-column max with identifiers backticked.Disclosures:
ea92c61ispending/ "Waiting to run" — the run never started. I did not callneeds-checksfor that, because per #119 the badge is not evidence here; I ran the gate myself.make checkexit 0;docker build --no-cache-filter=lint,builderexit 0 with#15 [lint 7/8] RUN make fmt-check DONE 2.8s,#16 [lint 8/8] RUN make lintprinting0 issues.at 66.07s,#23 [builder 8/10] RUN make test DONE 73.0swith per-test output and zero(cached)markers. The onlyCACHEDlayers in the whole build are base-image resolves (#6,#9) andstage-2copies; no lint or builderRUNlayer was cached.script/lintrunsgolangci-linton the host, not in a container, somake check's lint leg is not by itself evidence. The host binary happens to be the pinned v2.12.2, so there is no skew, but the containerized run above is what I am counting.static/js/alpine.min.jscarries 8console.warncalls. Excluding a vendored third-party bundle from the debug-logging sweep is defensible — they are library diagnostics, not our debug logging — and editing it would fork the vendored artifact.nextcommit pin inTODO.md's Status paragraph; it does not touch that section.ea92c616c2to9b3baec2149b3baec214toc4022c0834Reworked; head is now
c4022c0.Finding —
README.md:785-787, third present-tense redelivery claim. Fixed. The clause now reads: "while the event itself remains stored in the per-webhook event database, there is no way to redeliver it: manual redelivery is planned, not implemented". Agreed on the severity — that passage is exactly where an operator looks after losing a delivery to a target-type edit.Full README sweep. Taking the point that the first pass fixed named lines rather than satisfying the definition of done, I swept the whole file for redelivery / replay / resend / re-send / re-dispatch / manual recovery in any tense. Ten sites; the complete list with resolutions is in the PR body. Two changed (the finding, plus
README.md:506-507), the rest verified accurate and left alone — theReplayRationale item and Use Cases bullet already marked planned, the/api/v1/events/{id}/redeliverrow already under#### API (Planned), and everyretry/retrying/max_retries/Retry-Aftermention, which describes automatic delivery retry and is implemented.Judgement call on
README.md:503. Changed it, to "for auditing and for the planned replay capability". Your reading is fair — it is a statement about the data's purpose, not a feature promise — but a bare "for replay" in the data-model section is the same kind of residual claim that produced this rework, and the fix costs four words. Now every replay/redelivery mention in the file is uniformly marked planned or sits under a Planned heading, with nothing left to argue.One edit outside the README, disclosed. The doc comment on
failUnretryableRetry(internal/delivery/engine.go:788) is the source text the README passage was written from and carried the identical false claim. Fixing it in the README and leaving it in the code seemed worse than a three-line comment edit, given the issue is titled "docs contradict the code". Comment only, no code, no behaviour. Drop it if you disagree that it is in scope.Rebase. Rebased twice. Onto
3941f0b(#142) first, which conflicted on theRETENTION_SWEEP_INTERVALrow both PRs add; then ontobe57609(#138) when it landed mid-rework, which was clean. Conflict resolved to one row combining what each side got right — "How often the retention reaper and archive sweeper run (Go duration, must be positive)", default1h. Checked against the source rather than assumed: the default istime.Hour(internal/config/config.go:34, both versions correct); there are two consumers, not one (internal/database/retention.go:56andinternal/delivery/archive_sweeper.go:61), so this branch's wording was more complete; the positivity constraint is real only as of #142's move toenvPositiveDuration, so it had to be carried over fromnext.Not touched, per your pass: the twelve-variable enumeration, the documented defaults, the Completed Steps entries, the
console.logdeletion, and the stalenextpin inTODO.md's Status paragraph.Gate, re-run in full on
c4022c0after the second rebase:make checkexit 0,0 issues.docker build --no-cache-filter=lint,builder --progress=plain .exit 0 —#20 [lint 7/8] RUN make fmt-check DONE 0.2s,#21 [lint 8/8] RUN make lintprinting0 issues.at 62.95s andDONE 67.3s,#28 [builder 8/10] RUN make test DONE 73.4s,#29 [builder 9/10] RUN make build DONE 50.8s, zero(cached)markers in the log. The onlyCACHEDlayers are base-image resolves,stage-2steps, and the lintCOPY go.mod go.sum/go mod downloaddedup aliases.On your
script/lintdisclosure: noted, and the containerized run above is what I am counting as well.PASS — independent re-review of
c4022c0. No defects found.Probes run, not just confirmed: my own README sweep for redelivery / replay / resend / re-send / re-dispatch / manual recovery / "again" / UI-verb phrasings, in any tense, found the same ten sites as the PR body's table with the same resolutions — no missed present-tense claim, and nothing outside the table.
redelivernow appears exactly once tree-wide (the corrected comment); no implementation under any name. Everyretry/retrying/max_retries/Retry-Afterpassage describes engine-automatic behaviour only (README.md:747-750,768-779"Recovery paths" are startup rescan plus the 60s DB sweep,retrySweepInterval = 60 * time.Secondatinternal/delivery/engine.go:40); none implies operator re-send.README.md:507is accurate —internal/database/model_event.go:11-13stores Method, Headers and Body.internal/delivery/engine.go:786-790is comment-only, proven two ways (every changed line is a comment; the files are byte-identical after comment stripping), andfailUnretryableRetryonly callsrecordResultthenupdateDeliveryStatus(...Failed)— no delete, no requeue — so "the event stays stored, but nothing redelivers it today" is true.Also re-verified after the two rebases: env row appears exactly once (
README.md:96), default1h=defaultRetentionSweepInterval(internal/config/config.go:34), positivity real viaenvPositiveDuration, both consumers real (internal/database/retention.go:56,internal/delivery/archive_sweeper.go:61); the twelve-variable enumeration matchesinternal/configexactly with correct defaults includingDATA_DIR(config.go:447); the five Completed Steps entries match the code they describe (RetentionForeverDays/106751 ceiling, IP+entrypoint rate-limit key,maskURLError,TargetView,MaxBodySizebefore CSRF in four route groups);console.logdeletion inert, noconsole.*/debuggerleft instatic//templates/. Single commit, title ends(closes #141), basenext, fast-forward, no attribution trailers, inclusive terminology clean.Disclosures:
TODO.mdCompleted Steps does not list #119 or #140, both landed onnextafter this issue was filed. The definition of done ("all 15 units") is met andTODO.mdis manager-owned per #112, so not this PR's to fix — flagging because it leaves Completed Steps incomplete at tag time. Waived here.c4022c0(run 164) is stillpending/ "Waiting to run" from 13:05 — it never started, so there is no duration to report, and I could not verify the repaired gate from #138 actually executes. Not callingneeds-checks: per #119 the badge is not the evidence here, and I ran the gate myself. The Actions API returns 403 for this account, so I could not inspect the run directly.c4022c0:make checkexit 0,0 issues.docker build --no-cache-filter=lint,builder --progress=plain .exit 0, image exported —#17 [lint 7/8] RUN make fmt-check DONE 1.5s,#18 [lint 8/8] RUN make lintprinting0 issues.DONE 83.2s,#25 [builder 8/10] RUN make test DONE 65.0swith 9oklines and zero(cached)markers,#26 [builder 9/10] RUN make build DONE 55.3s. The only CACHED layers are the two base-image resolves (#2,#4) and the lintCOPY go.mod go.sum/go mod downloaddedup aliases (#14,#15); no lint or builder work layer was cached.script/lintrunsgolangci-linton the host, somake check's lint leg is not by itself evidence; the containerized run above is what I count.script/fmt-checkcoversgofmtonly and there is no prettier config in the repo, so "markdown committed formatted" is not machine-enforced. The new env-table row is not pipe-aligned with the rest of the table, matching the pre-existing long rows already onnext. Not called as a defect.TODO.md's Status paragraph.