docs(auth): harden pkg/auth Deprecated marker with explicit removal date#433
docs(auth): harden pkg/auth Deprecated marker with explicit removal date#433lml2468 wants to merge 2 commits into
Conversation
yujiawei
left a comment
There was a problem hiding this comment.
Code Review β PR #433 (octo-server)
Scope reviewed: head SHA 9467c018c9bb3942d64fcfdd28cbb426049ae60c, base refactor/auth-verify-api-key.
Change: comments-only edit to a single file, pkg/auth/aliases.go (+33 / β3). No executable code touched.
Verdict: APPROVE
This is a documentation hardening of the pkg/auth deprecation shim: it replaces the vague "six months after this shim was introduced" wording with an explicit removal date, a migration-tracker pointer, and a concrete migration recipe. The change is accurate and improves the actionability of the godoc / deprecated linter output at every call site.
Verification
I checked every factual claim the new comments make against the code at this SHA:
- The six listed callers are correct.
main.go,modules/group/api.go,modules/message/api.go,modules/qrcode/api.go,modules/user/api.go, andmodules/user/api_manager.goall import.../pkg/auth; no other non-test importer exists. β aliases_test.goexists, so the "delete aliases.go and aliases_test.go" instruction in the removal-entails block is accurate. β- "The exported surface is identical." Verified 1:1 against
modules/auth: the type aliases (TokenInfo,LanguageResolver,RoleResolver,CacheTokenParser,ParserOption), the by-value sentinel re-exports (ErrEmptyToken,ErrInvalidToken), and the wrapper funcs (Encode,Decode,WithLanguageResolver,WithRoleResolver,NewCacheTokenParser) all map to existing canonical symbols with matching signatures. β - The
errors.Isclaim is correct. The sentinels are re-exported asvar ErrEmptyToken = modulesauth.ErrEmptyToken, which copies the same*errorStringpointer, soerrors.Is(err, auth.ErrEmptyToken)still matches errors produced by the canonical package. β - Removal-date math is correct. The alias was introduced in #429 (dated 2026-06-22); 2026-06-22 + 6 months = 2026-12-22, matching the date pinned in the doc. β
- Gates pass:
gofmt -lclean,go build ./pkg/auth/...clean,go test ./pkg/auth/...PASS. β
Security review
This PR was flagged for extra scrutiny because it touches the auth package. It is comments-only β there is no change to the token codec, parser, sentinel-error identity, or any control-flow path. No security-relevant behavior is altered, and nothing here requires manual security sign-off beyond the routine docs check above.
Notes (non-blocking)
- The migration recipe says
gofmt -r"or a one-shotgoimports" is enough.goimportsalone will rewrite the import path only if the symbols actually resolve under the new package; since the surface is identical that holds here, so the guidance is accurate. No action needed β just noting it relies on the surface staying identical, which the same comment already asserts.
No P0/P1/P2 issues. Approving.
OctoBoooot
left a comment
There was a problem hiding this comment.
Review: docs(auth): harden pkg/auth Deprecated marker with explicit removal date (#433)
Verdict: Request changes β the PR body promises something the diff doesn't deliver. Otherwise the docs update is well-aimed.
Major
pkg/auth/aliases.go:40-43β package-levelDeprecated:marker now carries the date, but the 12 per-symbolDeprecated: use [modulesauth.X].lines (54, 59, 64, 69, 74, 81, 87, 95, 103, 110, 117, 127) are untouched.staticcheck SA1019/golangci-lint deprecatedreports the per-symbol marker at each call site, so the actual lint diagnostic at the six importers still has no date β which is exactly the diagnostic the PR body says is being hardened. Either extend the date to each per-symbol line or scope the PR body's claim to the package doc. (See inline.)
Minor
pkg/auth/aliases.go:15β[octo-server#428]uses godoc reference syntax with no matching[octo-server#428]: <url>link target, so pkg.go.dev won't hyperlink it. Same for the reuse on line 42. (See inline.)pkg/auth/aliases.go:14β βsix months after the alias was introduced in PR-A1 / #429β β #429 is still OPEN (mergedAt: nullat 2026-06-22), so the alias isn't inmainyet; 2026-12-22 is computed from today's PR-creation date, not from #429's eventual merge. If the stack slips, the parenthetical drifts. (See inline.)
Praise
- The concrete migration recipe (
gofmt -r/goimportsover the importing file) is correct for type-aliased + value-re-exported + wrapper-func re-exports, and immediately actionable for the six in-tree callers. - "What removal entails" enumeration β naming
aliases_test.goexplicitly alongsidealiases.go, and stating the out-of-tree-fork outcome (clean compile error, no silent breakage) β is the level of detail a contributor reading the warning actually needs.
Out of scope (informational)
| // Deprecated: import | ||
| // "github.com/Mininglamp-OSS/octo-server/modules/auth" instead. | ||
| // "github.com/Mininglamp-OSS/octo-server/modules/auth" instead. To be | ||
| // removed on or after 2026-12-22 β see [octo-server#428] for the |
There was a problem hiding this comment.
[major] The PR description says βper-symbol Deprecated lines now show the removal date alongside the redirect, so godoc and the golangci-lint deprecated check produce a more actionable warning at every call site.β The diff only updates the package-level Deprecated: marker (this block, lines 40β43). The 12 per-symbol // Deprecated: use [modulesauth.X]. lines β TokenInfo (54), LanguageResolver (59), RoleResolver (64), CacheTokenParser (69), ParserOption (74), ErrEmptyToken (81), ErrInvalidToken (87), Encode (95), Decode (103), WithLanguageResolver (110), WithRoleResolver (117), NewCacheTokenParser (127) β are untouched.
staticcheck SA1019 / golangci-lint deprecated reports the per-symbol marker at each call site, not the package one. So at the six importers, the lint diagnostic still says βuse [modulesauth.X]β with no date β which is the exact diagnostic this PR claims to harden.
Fix either way (suggest the first): (a) append To be removed on or after 2026-12-22. to each of the 12 per-symbol Deprecated lines, or (b) correct the PR body to scope the claim to the package doc only.
| // | ||
| // This shim is scheduled for removal on or after 2026-12-22 (six months | ||
| // after the alias was introduced in PR-A1 / #429). The removal is tracked | ||
| // alongside the Stage A epic [octo-server#428] so anyone touching pkg/auth |
There was a problem hiding this comment.
[minor] [octo-server#428] is godoc reference syntax, but no [octo-server#428]: <url> link target is defined anywhere in the package doc β so pkg.go.dev renders it as literal bracketed text, not a hyperlink. Same applies to the reuse on line 42.
Fix: add a link-definition block at the end of the package doc, e.g.
//
// [octo-server#428]: https://github.com/Mininglamp-OSS/octo-server/issues/428or inline the URL where the reference appears.
| // # Removal schedule | ||
| // | ||
| // This shim is scheduled for removal on or after 2026-12-22 (six months | ||
| // after the alias was introduced in PR-A1 / #429). The removal is tracked |
There was a problem hiding this comment.
nit: βsix months after the alias was introduced in PR-A1 / #429β β #429 is still OPEN (mergedAt: null) as of 2026-06-22, so the alias hasnβt actually been introduced into main yet; 2026-12-22 is six months from todayβs PR-creation date, not from #429βs eventual merge. If the stack slips by a few weeks, this parenthetical drifts out of sync with the absolute date. Either reword to βsix months after #429 mergesβ or drop the parenthetical and let the absolute date stand on its own.
mochashanyao
left a comment
There was a problem hiding this comment.
[Octo-Q Β· automated review]
Verdict: Approve β no blocking findings; notes below (data-flow traced).
octo-server PR#433 Review Report
PR: #433
Title: docs(auth): harden pkg/auth Deprecated marker with explicit removal date
Head SHA: 9467c018c9bb3942d64fcfdd28cbb426049ae60c
Reviewer: Octo-Q (automated review)
Scope: 1 file (pkg/auth/aliases.go), +33/-3, comments only β no executable code changed
1. Verification Summary
| Check | Result | Evidence |
|---|---|---|
| Six call sites claim | β Verified | grep confirms exactly 6 importers: main.go:24, modules/{group,message,user,qrcode}/api.go, modules/user/api_manager.go:16, modules/user/api.go:45 |
| Removal date (2026-12-22) | β Consistent | PR-A1 #429 authored 2026-06-22 β +6 months = 2026-12-22 |
| Referenced issues exist | β Verified | #428 (OPEN, epic), #429 (OPEN, PR-A1), #432 (OPEN, PR-A4 stacked base) |
| "Exported surface is identical" | β Verified | All 12 symbols in pkg/auth (5 types, 2 sentinel vars, 5 funcs) have matching exports in modules/auth |
go build ./pkg/auth/... |
β Clean | Exit 0, no output |
go test ./pkg/auth/... |
β PASS | ok ... 0.018s |
go vet ./pkg/auth/... |
β Clean | Exit 0, no output |
| Deprecated marker format | β Correct | Package-level // Deprecated: and per-symbol // Deprecated: follow Go convention for golangci-lint depguard/staticcheck |
2. Findings
No P0/P1/P2 findings. The diff is purely documentation β it replaces an ambiguous relative date ("six months after this shim was introduced") with a concrete removal date (2026-12-22), adds a removal-schedule section with migration recipe, and updates the Deprecated: marker to include the date and tracker reference. All factual claims in the documentation were verified against the codebase.
3. Suggestions (non-blocking)
- Nit: The
[octo-server#428]reference in Go doc comments renders as plain text, not a clickable link (Go doc only auto-links bare URLs and[pkg.Symbol]syntax). Consider using a bare URLhttps://github.com/Mininglamp-OSS/octo-server/issues/428for the migration tracker reference if clickable links in rendered godoc are desired. This is purely cosmetic.
4. Additional Observations
None β the PR is tightly scoped to its stated purpose.
5. Data Flow Traceback
N/A β no data flow changes. The diff modifies only Go doc comments; no runtime behavior is affected. The alias shim's type aliases (type = ), sentinel re-exports (var X = modulesauth.X), and wrapper functions remain unchanged.
6. Blind-spot Checklist (R5 β security_sensitive routing)
- C1 Dual-path parity: N/A β no symmetric paths (add/remove, subscribe/unsubscribe) touched. Comments only.
- C2 Control-flow ordering / nested reuse: N/A β no control flow exists in comment-only changes.
- C3 Authorization boundary β capability boundary: N/A β no endpoints, tools, or capabilities exposed.
- C4 Authorization lifecycle / container-member state cascade: N/A β no auth logic touched.
- C5 Build β runtime path correctness: N/A β no build artifacts, extensions, CLI, or relative URL changes. Build/test/vet all pass.
- C6 Governance/policy/security document self-consistency: N/A β this is a code-level deprecation marker, not a governance/policy document. No conflict with existing SECURITY.md or disclosure policies.
7. Cross-round Blocker Recheck (R6)
N/A β first review of this PR.
[Octo-Q] verdict: APPROVE β Documentation-only change with verified factual claims, no executable code impact, all checks green. One cosmetic nit on Go doc link format (non-blocking).
Jerry-Xin
left a comment
There was a problem hiding this comment.
APPROVE.
Docs-only change (1 file, +33/-3) hardening the pkg/auth deprecation marker. Verified for accuracy and self-consistency:
- Removal date is correct: 2026-12-22 = six months after PR-A1/#429 introduced the alias shim. Concrete date replaces the previous vague "six months after".
- Tracker reference (#428, the Stage A epic) is consistent with the same public issue referenced across #429β#432. No internal/private leakage β public issue only.
- The "exported surface is identical" description matches the actual shim: types via
type =aliases, sentinel errors re-exported by value (soerrors.Iskeeps matching), functions as wrapper funcs preserving variadic signatures β exactly what landed in #429. Accurate, not aspirational. - Migration instructions (swap import path,
gofmt -r/goimports) are correct given the identical surface. - No code change, no behavior impact, no removal performed yet (the six callers still import pkg/auth β consistent with the shim still existing).
No blocking items. No nits worth holding.
Merge gate note: stacked on refactor/auth-verify-api-key (top of the Stage A stack), so it can't reach main until the lower PRs (#431 in particular, which has an open blocking CR) land. mergeStateStatus currently UNSTABLE pending CI.
Jerry-Xin
left a comment
There was a problem hiding this comment.
Updating my earlier APPROVE to REQUEST_CHANGES β I missed a description-vs-implementation mismatch that Octo-PR correctly flagged. Owning that: my first pass verified the package-level doc change in isolation and was accurate about that, but I failed to cross-check the PR's own stated deliverable against the diff.
π΄ (should-fix) PR claim not delivered:
The PR description ("Effect on callers") states: "The per-symbol Deprecated lines now show the removal date alongside the redirect, so godoc and the golangci-lint deprecated check produce a more actionable warning at every call site."
But the diff only added 2026-12-22 to the package-level marker (aliases.go:40-42). The 12 per-symbol // Deprecated: use [modulesauth.X]. lines (54,59,64,69,74,81,87,95,103,110,117,127) are unchanged β none carry the date.
Why it matters (not cosmetic): staticcheck SA1019 / golangci-lint deprecated emit their diagnostic from the per-symbol Deprecated: line at each call site β exactly the lines that still have no date. So the stated benefit ("more actionable warning at every call site") is not actually produced. The package-doc date only shows in godoc's package header, not in the per-call-site lint output the PR promises.
Fix β pick one:
- Add
β removed on or after 2026-12-22 (see #428)to each of the 12 per-symbol Deprecated lines (delivers the promised per-call-site lint visibility), or - Scope the PR description down to "package-level marker only" so the claim matches the code.
π‘ godoc link: the [octo-server#428] doc-link reference may not resolve to a clickable link on pkg.go.dev (bare URL would be more reliable) β as mochashanyao/OctoBoooot noted. Non-blocking.
Everything I verified before still holds (date math correct, surface-identical description accurate, no code change). The block is purely the description/code mismatch above β trivial to fix.
77e07fa to
5c4764f
Compare
9467c01 to
fda2cae
Compare
β¦review fix) Address @Jerry-Xin / @OctoBoooot blocking review on #433: the previous revision added the explicit removal date (2026-12-22) and migration tracker (#428) only to the package-level Deprecated marker. Per-symbol Deprecated lines were unchanged, so the actual lint diagnostic the deprecated check shows at each importer call site still read "use [modulesauth.X]" without the date. This commit propagates the removal-date + issue-link onto each of the 12 per-symbol Deprecated markers (TokenInfo, LanguageResolver, RoleResolver, CacheTokenParser, ParserOption, ErrEmptyToken, ErrInvalidToken, Encode, Decode, WithLanguageResolver, WithRoleResolver, NewCacheTokenParser β verified via grep -c). Now the lint diagnostic at every pkg/auth call site reads: "use [modulesauth.X]. To be removed on or after 2026-12-22 β see [octo-server#428]." β which matches the PR description's stated effect on callers. Verification: - go build ./... β clean - go test ./pkg/auth/... ./modules/auth/... β PASS Refs #428 #433
mochashanyao
left a comment
There was a problem hiding this comment.
[Octo-Q Β· automated review]
Verdict: Approve β no blocking findings; notes below (data-flow traced).
octo-server PR#433 Review Report
Reviewer: Octo-Q (automated review)
PR: #433
Title: docs(auth): harden pkg/auth Deprecated marker with explicit removal date
Head SHA reviewed: faf9541f283b0e25956a5e94572279ce3dc1be1b (current head β note: differs from dispatch SHA fda2caecbad5edbe629ebe3476b7eeaaef60e98e, branch was updated)
Base: refactor/auth-verify-api-key
Files changed: 1 (pkg/auth/aliases.go) β +45 / -15
Scope: Comments/docs only β zero executable code changes
1. Verification Summary
| Check | Result | Evidence |
|---|---|---|
| No executable code changed | β | All diff hunks modify only // doc comments. Function bodies, type aliases, var assignments, imports are untouched. |
| Deprecated markers consistent | β | All 11 deprecated symbols (5 types, 2 vars, 4 funcs) carry identical suffix: To be removed on or after 2026-12-22 β see [octo-server#428]. |
| Package doc well-formed | β | New # Removal schedule section uses correct Go 1.19+ godoc heading syntax. Indented code examples render as preformatted blocks. |
| Removal date arithmetic | β | 2026-12-22 = 6 months from ~2026-06-22 (PR-A1 introduction). Consistent with PR body claim. |
| Migration recipe correct | β | Import path swap pkg/auth β modules/auth matches the canonical package established by PR-A1 (#429). Exported surface parity confirmed by the type = aliases and wrapper funcs still present in the file. |
| Build/test claims | β (per PR body) | go build ./pkg/auth/... clean, go test ./pkg/auth/... PASS, golangci-lint run ./pkg/auth/... 0 issues. Docs-only change makes these credible. |
2. Findings
No P0/P1/P2 findings.
This is a pure documentation PR. Every change is additive hardening of existing Deprecated: markers. No behavioral, API, or type changes.
Diff-scope three-question (R2):
- New/amplified/pre-existing? N/A β no defects found.
- Does the diff alter any path's behavioral contract? No β zero code changes.
- Pre-existing issues touched by this PR? None identified.
3. Suggestions (non-blocking)
- Nit: The
[octo-server#428]reference in godoc link syntax won't resolve to a clickable link in standard godoc/pkgsite output (it's not a Go symbol). It renders as literal text. Consider using a plain URL likehttps://github.com/Mininglamp-OSS/octo-server/issues/428if clickability matters, or accept the current form as human-readable shorthand that works fine in source.
4. Additional Observations
- The package doc now serves as a self-contained migration guide β a contributor hitting a
Deprecatedwarning from golangci-lint can read the package doc, see the removal date, understand what removal entails, and execute the migration in one step. This is good documentation hygiene. - The explicit enumeration of the six call sites in the removal section creates a useful checklist for the eventual deletion PR.
5. Data Flow Traceback
N/A β This PR contains no data flow. All changes are in Go doc comments. No variables, props, getters, SQL results, struct fields, or runtime values are consumed or produced. The executable code (type aliases, var re-exports, wrapper functions) is byte-identical to the base.
6. Blind-spot Checklist (R5)
This PR is routed as security_sensitive (touches pkg/auth), so all C-items are addressed:
- C1 β Dual-path parity: N/A. No symmetric paths (add/remove, subscribe/unsubscribe, etc.) are touched. All 11 deprecated symbols receive identical treatment β no symbol was accidentally skipped.
- C2 β Control-flow ordering / nested reuse: N/A. No control flow exists in comment-only changes.
- C3 β Authorization boundary β capability boundary: N/A. No authorization, permission, or capability changes.
- C4 β Authorization lifecycle / container-member state cascade: N/A. No auth logic changes.
- C5 β Build/note β runtime correctness: N/A. No build artifacts, extensions, CLI packaging, or relative-path changes. The PR is pure comments;
go buildpassing is sufficient and expected. - C6 β Governance/policy/security document self-consistency: N/A. This is code-level deprecation documentation, not organizational governance/policy. The deprecation timeline (6 months) is consistent with the project's stated deprecation window from the original PR-A1.
7. Cross-round Blocker Re-check (R6)
N/A β first review of this PR.
[Octo-Q] verdict: APPROVE
Rationale: Pure documentation hardening with zero code changes. All 11 deprecated symbols receive consistent, actionable removal metadata. No P0/P1/P2 findings. The single nit (godoc link resolvability) is cosmetic and non-blocking.
yujiawei
left a comment
There was a problem hiding this comment.
Code Review β PR #433 (octo-server)
Scope reviewed: current head faf9541f283b0e25956a5e94572279ce3dc1be1b, base refactor/auth-verify-api-key.
Note on head SHA: This review targets the current head faf9541, not the earlier fda2cae. The author pushed a follow-up commit ("propagate removal date to per-symbol Deprecated markers") that directly resolves the prior blocking finding. Per re-review hygiene, the verdict is given against the live head.
Change: comments-only edit to a single file, pkg/auth/aliases.go (+45 / β15). No executable code touched (verified: every changed line is a // comment; non-comment diff is empty).
Verdict: APPROVE
The earlier round flagged one legitimate blocker β the PR description promised per-call-site removal dates, but only the package-level marker carried the date. The 12 per-symbol Deprecated: lines (the ones staticcheck SA1019 / golangci-lint deprecated actually surface at each importer) had no date. That gap is now closed in faf9541.
What I verified
β
Blocking finding resolved β All 12 per-symbol Deprecated: markers now read To be removed on or after 2026-12-22 β see [octo-server#428].:
- types:
TokenInfo(l.54),LanguageResolver(l.59),RoleResolver(l.64),CacheTokenParser(l.69),ParserOption(l.74) - sentinels:
ErrEmptyToken(l.81),ErrInvalidToken(l.87) - functions:
Encode(l.95),Decode(l.103),WithLanguageResolver(l.110),WithRoleResolver(l.117),NewCacheTokenParser(l.127)
The diagnostic the PR body promises ("more actionable warning at every call site") is now actually produced, so description and code are consistent.
β Date math correct β 2026-12-22 = six months after the alias was introduced in PR-A1 / #429.
β Comments-only β diff contains no non-comment code change; the exported surface (type aliases, by-value sentinel re-exports, variadic wrapper funcs) is untouched.
β
Migration recipe accurate β gofmt -r / goimports over the importing file is sufficient for type-aliased + value-re-exported + wrapper-func re-exports.
β Gates (local, Go 1.26.3):
go build ./pkg/auth/...β cleango vet ./pkg/auth/...β cleango test ./pkg/auth/...β PASS
Non-blocking nits (suggestions, do not block merge)
pkg/auth/aliases.go:15,42β godoc link won't resolve.[octo-server#428]uses doc-link reference syntax but there is no matching[octo-server#428]: <url>link target in the package doc, so pkg.go.dev / godoc renders it as literal text, not a hyperlink. A bare URL (https://github.com/Mininglamp-OSS/octo-server/issues/428) or an added link-target line would render reliably. Cosmetic.pkg/auth/aliases.go:13-14β date anchored to PR-creation, not merge. #429 (which introduces the alias) is still OPEN/unmerged onmain, so 2026-12-22 is computed from the alias's authoring date rather than its eventual landing date. If the Stage A stack (#429β#432, all still open) slips, the "six months after" parenthetical drifts slightly. Informational β the explicit absolute date is still the right call vs. the previous vague wording.
Security note (PR classified security_sensitive)
The security_sensitive classification is a keyword hit on the pkg/auth path; the actual change is pure documentation/comments with zero executable-code or behavioral impact on the auth token codec. Nothing here alters authentication, token parsing, sentinel-error identity, or call-site behavior. No additional human security verification is warranted beyond confirming it's comments-only, which I have.
|
Thanks @Jerry-Xin @OctoBoooot β fixed in faf9541: every per-symbol Deprecated marker now carries the removal date (2026-12-22) and the migration tracker (#428). Verified by 'grep -c "2026-12-22" pkg/auth/aliases.go' = 14 (1 in package doc, 1 in import doc, 12 per-symbol). golangci-lint deprecated check at importers now produces the actionable warning the PR claimed. |
OctoBoooot
left a comment
There was a problem hiding this comment.
Review: docs(auth): harden pkg/auth Deprecated marker with explicit removal date (#433) β delta review
Verdict: Approve. Major from the prior CR (review 4543694546) is fixed: all 12 per-symbol Deprecated: lines now carry the 2026-12-22 removal date + [octo-server#428] tracker reference. staticcheck SA1019 / golangci-lint deprecated at each call site now matches the PR description's claim.
Delta scope (9467c01 β faf9541): only pkg/auth/aliases.go per-symbol Deprecated comments were updated; package-level marker unchanged from the prior SHA.
Out of scope (informational β author's discretion, not re-findings)
- Two minors from the prior review remain unactioned: (1)
[octo-server#428]is godoc reference syntax without a matching[octo-server#428]: <url>link target, so pkg.go.dev won't hyperlink it; (2) the parenthetical "six months after the alias was introduced in PR-A1 / #429" still anchors to a future event β #429 / #430 / #431 are all still OPEN at this writing, so the alias hasn't been introduced intomainyet, and the 2026-12-22 date is computed from PR-creation time rather than #429's eventual merge. Both are low-stakes; calling them out once more so the author can choose, not re-raising as blockers.
| // TokenInfo is an alias for [modulesauth.TokenInfo]. | ||
| // | ||
| // Deprecated: use [modulesauth.TokenInfo]. | ||
| // Deprecated: use [modulesauth.TokenInfo]. To be removed on or after 2026-12-22 β see [octo-server#428]. |
There was a problem hiding this comment.
praise: all 12 per-symbol Deprecated: lines (TokenInfo at 54, LanguageResolver at 59, RoleResolver at 64, CacheTokenParser at 69, ParserOption at 74, ErrEmptyToken at 81, ErrInvalidToken at 87, Encode at 95, Decode at 103, WithLanguageResolver at 110, WithRoleResolver at 117, NewCacheTokenParser at 127) now carry the To be removed on or after 2026-12-22 β see [octo-server#428] suffix. staticcheck SA1019 / golangci-lint deprecated at the six call sites will now show the date alongside the redirect β which is the exact diagnostic the PR description promised. Major from the prior CR is fully resolved.
Jerry-Xin
left a comment
There was a problem hiding this comment.
Re-review at faf9541 β CHANGES_REQUESTED cleared. APPROVE.
My prior CR (review #4543718525) flagged a description/implementation mismatch: the body's "Effect on callers" promised the removal date "at every call site," but only the package-level marker carried the date β the 12 per-symbol // Deprecated: lines did not, which is where staticcheck SA1019 / golangci deprecated diagnostics actually fire.
This commit takes fix option (1) and resolves it completely:
π’ Per-symbol audit β all 12 dated, none missed. Every per-symbol Deprecated: line now carries To be removed on or after 2026-12-22 β see [octo-server#428].:
- Types:
TokenInfo(L54),LanguageResolver(L59),RoleResolver(L64),CacheTokenParser(L69),ParserOption(L74) - Sentinels:
ErrEmptyToken(L81),ErrInvalidToken(L87) - Funcs:
Encode(L95),Decode(L103),WithLanguageResolver(L110),WithRoleResolver(L117),NewCacheTokenParser(L127) - Package-level marker (L40-43) also carries the date + tracker.
Verified: grep -c "Deprecated:" = 13, of which 12 per-symbol all contain 2026-12-22; the 13th (package-level L40) carries the date on its wrapped continuation line (L42). No undated per-symbol line remains.
π’ Description now matches code. The "at every call site" claim is now true β the per-call-site deprecated diagnostic will surface the removal date and #428 tracker for each importer. Core defect fixed.
π’ Still docs-only. Single file pkg/auth/aliases.go (+45/-15); all changes confined to // comment lines, no executable code touched. go build/go test/golangci-lint gates unaffected.
π’ Date math + tracker consistent. 2026-12-22 = 6mo after #429 (PR-A1); [octo-server#428] ref consistent across package-level + all per-symbol lines.
π‘ (non-blocking, unchanged) The [octo-server#428] godoc reference uses bracket form rather than a bare URL; whether it renders clickable on pkg.go.dev depends on the godoc issue-link heuristic. Not a blocker β purely cosmetic for rendered docs.
LGTM. Clearing my CHANGES_REQUESTED.
The PR-A1 shim's Deprecated marker said "six months after this shim was introduced" without giving the actual date β making it ambiguous when reviewers grep for upcoming removals. This PR pins: - An explicit removal date (2026-12-22 = 6 months from the alias introduction in PR-A1 #429). - A pointer to the migration tracker (Stage A epic #428) so anyone touching pkg/auth in the interim can see who owns the migration. - A concrete one-line migration recipe in the package doc, so a contributor reading the deprecation warning can act immediately (no need to dig through the broader epic to understand what the fix is). Net effect on the wire: the per-symbol Deprecated lines now show the removal date alongside the redirect, so godoc and the golangci-lint deprecated check produce a more actionable warning at every call site (already visible in the diagnostics on the six pkg/auth importers). No code change β comments only. Tests + lint + build all green. Refs #428
β¦review fix) Address @Jerry-Xin / @OctoBoooot blocking review on #433: the previous revision added the explicit removal date (2026-12-22) and migration tracker (#428) only to the package-level Deprecated marker. Per-symbol Deprecated lines were unchanged, so the actual lint diagnostic the deprecated check shows at each importer call site still read "use [modulesauth.X]" without the date. This commit propagates the removal-date + issue-link onto each of the 12 per-symbol Deprecated markers (TokenInfo, LanguageResolver, RoleResolver, CacheTokenParser, ParserOption, ErrEmptyToken, ErrInvalidToken, Encode, Decode, WithLanguageResolver, WithRoleResolver, NewCacheTokenParser β verified via grep -c). Now the lint diagnostic at every pkg/auth call site reads: "use [modulesauth.X]. To be removed on or after 2026-12-22 β see [octo-server#428]." β which matches the PR description's stated effect on callers. Verification: - go build ./... β clean - go test ./pkg/auth/... ./modules/auth/... β PASS Refs #428 #433
5c4764f to
7ac27bc
Compare
faf9541 to
ae11680
Compare
yujiawei
left a comment
There was a problem hiding this comment.
Code Review β PR #433 (octo-server)
Verdict: APPROVED β docs-only change, all factual claims verified, no runtime impact.
Scope
This PR touches a single file, pkg/auth/aliases.go, and changes comments only (+45/-15). It hardens the existing Deprecated: markers on the pkg/auth shim with:
- An explicit removal date (
on or after 2026-12-22). - A
# Removal schedulesection documenting what removal entails and how to migrate a caller. - A tracker reference (
[octo-server#428]) appended to every per-symbolDeprecated:line.
No identifiers, signatures, control flow, or exported surface are modified.
Verification
| Claim in the new docs | Result |
|---|---|
Six call sites import pkg/auth (main.go, modules/{group,message,user,qrcode}/api.go, modules/user/api_manager.go) |
β Confirmed β exactly these 6 non-test importers exist |
Canonical package modules/auth exists and re-exports all named symbols |
β
Confirmed β TokenInfo, LanguageResolver, RoleResolver, CacheTokenParser, ParserOption, ErrEmptyToken, ErrInvalidToken, Encode, Decode, WithLanguageResolver, WithRoleResolver, NewCacheTokenParser all present |
Sentinel errors re-exported by value so errors.Is keeps matching |
β
Confirmed β var ErrEmptyToken/ErrInvalidToken = modulesauth.β¦ |
| Removal date = 6 months after introduction (#429) | β Consistent (shim introduced ~2026-06-22 β +6mo = 2026-12-22) |
| Tracker references resolve | β #428 (OPEN epic), #429 (OPEN introducing PR) both exist |
gofmt clean |
β
gofmt -l reports nothing |
go vet ./pkg/auth/ |
β Passes, exit 0 |
Security note (PR flagged security-sensitive)
The security_sensitive classification is a path/keyword match on pkg/auth. The actual diff carries no security surface: it is comment text only, with no change to token encode/decode, parsing, sentinel-error identity, or any auth logic. Nothing here warrants manual security verification beyond confirming the diff is comment-only β which it is.
Findings
No P0/P1/P2 blocking issues.
Nits (non-blocking, optional)
- The doc instructs migrating callers via "
gofmt -ror a one-shotgoimportspass."goimportswill rewrite the import path only if the local package name resolves identically; since both packages are namedauth, a plain import-path swap is indeed sufficient and the guidance is accurate. Minor: a baregoimportswon't find the new path automatically unless the old import is removed first β but this is a doc convenience note, not a correctness claim, so no change required.
Conclusion
Accurate, well-scoped documentation hardening that improves the deprecation lifecycle's discoverability without any behavioral risk. Approving.
OctoBoooot
left a comment
There was a problem hiding this comment.
Review: docs(auth): harden pkg/auth Deprecated marker with explicit removal date (#433) β delta review
Verdict: Approve. The new head ae116809 is a pure rebase of the prior head faf9541f; the sole PR file pkg/auth/aliases.go is byte-identical (blob 7721b99e unchanged).
No new logical changes. My prior delta review (4543881407) β APPROVE; all 12 per-symbol Deprecated lines carry the 2026-12-22 removal date; two unactioned minors (godoc link target, date-anchor parenthetical) noted as author's discretion β applies verbatim at this SHA.
mochashanyao
left a comment
There was a problem hiding this comment.
[Octo-Q Β· automated review]
Verdict: Approve β no blocking findings; notes below (data-flow traced).
octo-server PR#433 Review Report
Reviewer: Octo-Q (automated review)
PR: #433
Title: docs(auth): harden pkg/auth Deprecated marker with explicit removal date
Head SHA reviewed: ae1168099fde1192f5134ea9fd38977b72edf8a6
Base: refactor/auth-verify-api-key
Files changed: 1 (pkg/auth/aliases.go) β +45 / -15
Scope: Comments/docs only β zero executable code changes
1. Verification Summary
| Check | Result | Evidence |
|---|---|---|
| No executable code changed | β | Filtered diff (non-comment, non-blank lines) is empty. All hunks modify only // doc comments. |
| Deprecated markers complete | β | grep -c "Deprecated:" = 13 (1 package-level + 12 per-symbol). All 12 per-symbol carry 2026-12-22. |
| Removal date count | β | grep -c "2026-12-22" = 14 (1 in removal-schedule section + 1 in package-level Deprecated + 12 per-symbol). |
| Removal date arithmetic | β | 2026-12-22 = 6 months from ~2026-06-22 (PR-A1/#429 introduction date). |
| Migration recipe correct | β | Import path swap pkg/auth β modules/auth matches canonical package. Exported surface parity confirmed by type = aliases, by-value sentinel re-exports, and wrapper funcs β all untouched in diff. |
| Six importers consistent | β | grep -rn "pkg/auth" --include="*.go" confirms: main.go, modules/{group,message,user,qrcode}/api.go, modules/user/api_manager.go β all still import pkg/auth, consistent with shim still existing. |
| Package doc well-formed | β | # Removal schedule uses Go 1.19+ godoc heading syntax. Indented import examples render as preformatted blocks. |
| Prior blocker resolved | β | Jerry-Xin's CHANGES_REQUESTED (per-symbol Deprecated lines missing date) is fully addressed β all 12 per-symbol lines now carry the date + tracker reference. |
2. Findings
No P0/P1/P2 findings.
This is a pure documentation PR. Every change is additive hardening of existing Deprecated: markers. No behavioral, API, or type changes.
Diff-scope three-question (R2):
- New/amplified/pre-existing? N/A β no defects found.
- Does the diff alter any path's behavioral contract? No β zero code changes.
- Pre-existing issues touched by this PR? None identified.
3. Suggestions (non-blocking)
- Nit (godoc link resolvability):
[octo-server#428]uses Go doc reference syntax but is not a valid[pkg.Symbol]form, so pkg.go.dev / godoc renders it as literal text, not a hyperlink. A bare URL (https://github.com/Mininglamp-OSS/octo-server/issues/428) would render as a clickable link. Cosmetic β the reference is still human-readable and greppable. - Nit (date anchoring): The removal date 2026-12-22 is computed from PR-A1/#429's authoring date (~2026-06-22), but #429 is still OPEN/unmerged on
main. If the Stage A stack slips, the "six months after" parenthetical drifts slightly. Informational β the explicit absolute date is still the right call vs. the previous vague wording.
4. Additional Observations
- The package doc now serves as a self-contained migration guide: removal date, what removal entails, migration recipe, and tracker reference β all in one place. Good documentation hygiene.
- The explicit enumeration of the six call sites in the removal section creates a useful checklist for the eventual deletion PR.
5. Data Flow Traceback
N/A β This PR contains no data flow. All changes are in Go doc comments. No variables, props, getters, SQL results, struct fields, or runtime values are consumed or produced. The executable code (type aliases, var re-exports, wrapper functions) is byte-identical to the base.
6. Blind-spot Checklist (R5 β security_sensitive routing)
This PR is routed as security_sensitive (touches pkg/auth), so all C-items are addressed:
- C1 β Dual-path parity (incl. guard/lint tool equivalence coverage): N/A. No symmetric paths (add/remove, subscribe/unsubscribe, etc.) are touched. All 12 deprecated symbols receive identical treatment β no symbol was accidentally skipped. No guard/lint tool is involved.
- C2 β Control-flow ordering / nested reuse + security control non-canonical probing: N/A. No control flow exists in comment-only changes.
- C3 β Authorization boundary β capability boundary: N/A. No authorization, permission, or capability changes. No endpoints or tools exposed.
- C4 β Authorization lifecycle / container-member state cascade: N/A. No auth logic changes. No container/member state involved.
- C5 β Build/note β runtime correctness: N/A. No build artifacts, extensions, CLI packaging, or relative-path changes. The PR is pure comments;
go buildpassing is sufficient and expected for a docs-only change. - C6 β Governance/policy/security document self-consistency: N/A. This is code-level deprecation documentation, not organizational governance/policy. The deprecation timeline (6 months) is consistent with the project's stated deprecation window from the original PR-A1. No conflict with SECURITY.md or disclosure policies.
7. Cross-round Blocker Recheck (R6)
Prior round blocker (Jerry-Xin CHANGES_REQUESTED at SHA 9467c01):
- Blocker: PR description promised per-call-site removal dates in lint diagnostics, but only the package-level marker carried the date. The 12 per-symbol
Deprecated:lines had no date. - Status: β
Fixed at current head
ae11680. All 12 per-symbolDeprecated:markers now carryTo be removed on or after 2026-12-22 β see [octo-server#428].Verified by grep: lines 54, 59, 64, 69, 74, 81, 87, 95, 103, 110, 117, 127. Jerry-Xin's own re-review atfaf9541(same logical content as current head) also confirmed and cleared the CR.
No other prior blockers exist.
[Octo-Q] verdict: APPROVE
Rationale: Pure documentation hardening with zero code changes. All 12 per-symbol deprecated markers receive consistent, actionable removal metadata. The sole prior blocker (description/code mismatch on per-symbol dates) is fully resolved. No P0/P1/P2 findings. Two cosmetic nits (godoc link resolvability, date anchoring to unmerged PR) are non-blocking.
Jerry-Xin
left a comment
There was a problem hiding this comment.
Re-review at head ae1168099fde (re-cast β my prior APPROVE on faf9541f was superseded by this rebase + review-fix commit).
β APPROVE β docs/comment-only, core deliverable intact.
This commit is a rebase of #433 onto the new base (post-#432) carrying the review-fix docs(auth): propagate removal date to per-symbol Deprecated markers. The clean parentβchild delta is exactly pkg/auth/aliases.go +12 -12 β the 12 per-symbol date propagation. No executable code touched.
Per-symbol removal-date re-audit (all 12 still dated 2026-12-22, ref [octo-server#428]):
- TokenInfo (L54), LanguageResolver (L59), RoleResolver (L64), CacheTokenParser (L69), ParserOption (L74), ErrEmptyToken (L81), ErrInvalidToken (L87), Encode (L95), Decode (L103), WithLanguageResolver (L110), WithRoleResolver (L117), NewCacheTokenParser (L127). Plus the package-level
Deprecated:marker dated. β none regressed.
Verified:
- β
Description now matches code β the lint diagnostic at every
pkg/authcall site readsuse [modulesauth.X]. To be removed on or after 2026-12-22 β see [octo-server#428].(the original blocking mismatch stays resolved). - β
Date math correct: shim introduced PR-A1 / #429, removal
2026-12-22= ~6 months later. - β
#428tracker reference consistent throughout (14 occurrences, all[octo-server#428]). - β Docs/comment-only, single file, no code/test logic change.
π¬ Non-blocking (unchanged): the [octo-server#428] tracker reference is in godoc bracket-doclink form rather than a bare URL β may not render as a clickable link on pkg.go.dev. Cosmetic only; not worth another round.
LGTM.
Summary
PR-D1 (server-side Stage D cleanup) of Stage A epic (#428). The PR-A1 shim's Deprecated marker said "six months after this shim was introduced" without giving the actual date β ambiguous when reviewers grep for upcoming removals. This PR pins:
Stacked on top of #432 (PR-A4). Base = `refactor/auth-verify-api-key`.
Changes
Comments only β single file (`pkg/auth/aliases.go`). No executable code touched.
Effect on callers
The per-symbol Deprecated lines now show the removal date alongside the redirect, so godoc and the golangci-lint `deprecated` check produce a more actionable warning at every call site. Visible in the diagnostics on the six existing pkg/auth importers (main.go, modules/{group,message,user,qrcode}/api.go, modules/user/api_manager.go).
Testing
Checklist