Skip to content

chore: correct false comments, remove dead code, wire missing telemetry - #779

Merged
lakhansamani merged 1 commit into
mainfrom
chore/comment-accuracy-and-dead-code
Aug 17, 2026
Merged

chore: correct false comments, remove dead code, wire missing telemetry#779
lakhansamani merged 1 commit into
mainfrom
chore/comment-accuracy-and-dead-code

Conversation

@lakhansamani

Copy link
Copy Markdown
Contributor

A repo-wide scan for two things: comments asserting behaviour the code does not
have, and exported identifiers nothing references. unused is already enabled in
.golangci.yml, but it only catches unexported identifiers within a package —
exported-but-unreferenced code in internal/ slips past it, which is where all of
this was hiding.

False claims corrected

Where Claim Reality
mcp/schema_test.go:69 documents TestSchemaForMessage_ScalarOnly function is TestSchemaForMessage_AllScalarKinds
interceptors_test.go:147 documents TestValidate_PreservesInvariant function is TestValidate_BuildsCleanly
memory_store/db/cache.go x3 "7 providers" / "all 7 backends" 6 — the 7th directory is provider_template, a scaffold

mcp/server.go's reference to TestServer_StdioOnly is deliberately left: it is
past tense ("the guard that enforced it"), and transport_test.go:17 documents the
replacement. Accurate history, not a stale claim.

Dead code removed

webhook_event_authorization.go — 13 constants. Worse than unused: nothing
fired them and nothing could subscribe to them. They were never added to
validators.validWebhookEventNames, so registering a webhook for
permission.created was rejected as an invalid event name. The file's own header
records this exact bug happening before with the SCIM events — same shape, second
occurrence.

GoogleUserInfoURL (already self-documented "deprecated and not used") and
MicrosoftUserInfoURL — both providers resolve via OIDC discovery now.

TokenTypeURNRefreshToken / TokenTypeURNIDToken — RFC 8693 URNs this server
does not accept (isSupportedExchangeTokenType takes access_token and jwt only).
The block now states what Authorizer accepts rather than copying the registry.

Missing telemetry wired

The constants existed; nothing emitted them. Same class of defect — a name
asserting behaviour never implemented.

  • AuditPasswordChangedEvent and AuditMFADisabledEvent now fire from
    UpdateProfile, in addition to AuditProfileUpdatedEvent so nothing
    consuming that today breaks. A password change and an MFA disable were
    indistinguishable from a display-name edit — anyone auditing "who changed a
    password" had nothing to query.
  • metrics.EventMagicLink and metrics.EventVerifyOTP are now recorded.
    Magic-link and OTP volume were invisible next to login/signup/oauth on the same
    dashboard. EventVerifyOTP was even asserted by a test while never emitted in
    production.

Deliberately not deleted

Nine audit-event constants remain unemitted. Each now says RESERVED and why,
instead of "is logged when X" — the "is logged when" was the false claim, not the
constant's existence.

AuditAdminUserCreatedEvent is the clearest. I first read it as a compliance gap
(update and delete are audited, create isn't) and that was wrong: there is no
admin create-user operation at all. AdminProvider exposes
Users/User/UpdateUser/DeleteUser, and creation goes through the invite flow, which
logs AuditAdminInviteSentEvent. Never a missing log call.

After this PR plus #776 and #777, every remaining zero-reference exported constant
is either explicitly RESERVED with a stated reason, or a
TokenEndpointAuthMethodClientSecretPost-style enum member used only in tests. Not
"zero unused code" — documented and accounted for, which is the honest goal.

Verification

TestProfileSecurityEventsAreAuditedSeparately confirmed failing with the source
reverted (no user.password_changed audit entry, no user.mfa_disabled audit entry).

go build ./...   OK
go vet ./...     OK
make test        exit 0 — 43 packages, 0 FAIL
make lint        exit 0

@lakhansamani
lakhansamani force-pushed the chore/comment-accuracy-and-dead-code branch from 34b367a to 9a75b89 Compare August 17, 2026 06:02
A repo-wide scan for comments asserting behaviour the code does not have,
and for exported identifiers nothing references.

False claims corrected:

- Two comments named a test that does not exist
  (TestSchemaForMessage_ScalarOnly, TestValidate_PreservesInvariant) —
  renamed to the functions they actually document. mcp/server.go's
  reference to TestServer_StdioOnly is left alone: it is past tense and
  transport_test.go documents the replacement.
- memory_store/db/cache.go claimed "7 providers" / "all 7 backends" in
  three places. There are 6; the 7th directory is provider_template, a
  scaffold. Every other comment in the repo, and AGENTS.md, says six.

Dead code removed:

- webhook_event_authorization.go: 13 constants for resource/scope/policy/
  permission events. Nothing fired them AND nothing could subscribe —
  they were never added to validators.validWebhookEventNames, so
  registering a webhook for one was rejected as invalid. The same file's
  header records this exact bug happening before with the SCIM events.
- GoogleUserInfoURL (already self-documented "deprecated and not used")
  and MicrosoftUserInfoURL: both providers resolve via OIDC discovery.
- TokenTypeURNRefreshToken / TokenTypeURNIDToken: RFC 8693 URNs this
  server does not accept (see isSupportedExchangeTokenType). The block
  now states what Authorizer accepts rather than copying the registry.

Missing telemetry wired — the constants existed, nothing emitted them:

- AuditPasswordChangedEvent and AuditMFADisabledEvent now fire from
  UpdateProfile, IN ADDITION to AuditProfileUpdatedEvent so nothing
  consuming that today breaks. A password change and an MFA disable were
  indistinguishable from a display-name edit.
- metrics.EventMagicLink and metrics.EventVerifyOTP are now recorded.
  Magic-link and OTP volume were invisible beside login/signup/oauth on
  the same dashboard; EventVerifyOTP was even asserted by a test while
  never being emitted in production.

Deliberately NOT deleted: nine audit-event constants that remain
unemitted. Each now says RESERVED and why, rather than "is logged when
X" — which was the false claim. AuditAdminUserCreatedEvent is the
clearest case: there is no admin create-user operation at all
(AdminProvider has Users/User/UpdateUser/DeleteUser; creation goes
through the invite flow), so it was never a missing log call.
@lakhansamani
lakhansamani force-pushed the chore/comment-accuracy-and-dead-code branch from 9a75b89 to 301e879 Compare August 17, 2026 06:09
@lakhansamani
lakhansamani merged commit 89b53bc into main Aug 17, 2026
1 check passed
@lakhansamani
lakhansamani deleted the chore/comment-accuracy-and-dead-code branch August 17, 2026 06:09
lakhansamani added a commit that referenced this pull request Aug 17, 2026
…po (#783)

The rule keeping e2e-playground's TLS fixtures tracked cited
docs/superpowers/plans/2026-07-20-playground-e2e.md as its justification.
That path is gitignored (.gitignore:29) and was never committed, so the
citation could not be followed by anyone cloning the repo — and the file
has since been deleted locally too.

States the reason inline instead. Same class as the stale references
corrected in #779; this one was missed because .gitignore is not a Go
file and the scan only walked *.go.
lakhansamani added a commit that referenced this pull request Aug 19, 2026
* docs(changelog): cover #773-#783

Unreleased linked 50 PRs and none of #773-#783, so every change made
after rc.22 - including four security fixes - was missing from the
CHANGELOG a user reads at 2.4.0.

Refs #773, #774, #775, #776, #777, #778, #779, #781, #782, #783

* chore: bump web/app to authorizer-react 2.2.0

authorizer-react 2.2.0 is published on authorizer-js 4.0.0; drop the
-rc.7 pin. Also stamps the CHANGELOG's Unreleased section as 2.4.0.

* test(e2e): make the authorizer host ports overridable

The seven authorizer services published fixed host ports, so the suite
could not run on a machine already using 8080-8086 - it failed at
"address already in use" before any test ran. The mock services already
take this shape. Playwright reaches every service by compose DNS, so the
host mapping is for humans only and the defaults are unchanged.
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