From 5cbe080d250a2d967a5c2041138b372d8d5fba8d Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Sun, 1 Nov 2026 18:27:36 +0100 Subject: [PATCH 1/3] docs: capture the log-invite-outcomes openspec change Planning artifacts for additive structured logging of every POST /invite branch (see proposal.md). Investigation tails the planner#3069 incident: the unmapped Slack error branch returned a 200 page and logged nothing, hiding inviter_email_not_verified for weeks. --- openspec/changes/archive/.gitkeep | 0 .../log-invite-outcomes/.openspec.yaml | 2 + .../changes/log-invite-outcomes/design.md | 45 +++++++++++ .../changes/log-invite-outcomes/proposal.md | 31 ++++++++ .../specs/invite-observability/spec.md | 74 +++++++++++++++++++ openspec/changes/log-invite-outcomes/tasks.md | 28 +++++++ openspec/config.yaml | 32 ++++++++ openspec/specs/.gitkeep | 0 8 files changed, 212 insertions(+) create mode 100644 openspec/changes/archive/.gitkeep create mode 100644 openspec/changes/log-invite-outcomes/.openspec.yaml create mode 100644 openspec/changes/log-invite-outcomes/design.md create mode 100644 openspec/changes/log-invite-outcomes/proposal.md create mode 100644 openspec/changes/log-invite-outcomes/specs/invite-observability/spec.md create mode 100644 openspec/changes/log-invite-outcomes/tasks.md create mode 100644 openspec/config.yaml create mode 100644 openspec/specs/.gitkeep diff --git a/openspec/changes/archive/.gitkeep b/openspec/changes/archive/.gitkeep new file mode 100644 index 0000000..e69de29 diff --git a/openspec/changes/log-invite-outcomes/.openspec.yaml b/openspec/changes/log-invite-outcomes/.openspec.yaml new file mode 100644 index 0000000..25002a8 --- /dev/null +++ b/openspec/changes/log-invite-outcomes/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-11-01 diff --git a/openspec/changes/log-invite-outcomes/design.md b/openspec/changes/log-invite-outcomes/design.md new file mode 100644 index 0000000..5cad3ab --- /dev/null +++ b/openspec/changes/log-invite-outcomes/design.md @@ -0,0 +1,45 @@ +# Design + +## Context + +`src/index.js` handles `POST /invite` with ten terminal branches. Six already log structured events (`secrets_missing`, `turnstile_verify_error`, `slack_unreachable`, `invite_failed`, plus the scheduled health-check events); four log nothing: turnstile-verification-failed, invalid-email, already-invited, and the unmapped-Slack-error branch that hid planner#3069 for weeks. Slack documents 44 error strings for the invite endpoint and has shown it adds new ones silently, so a fixed mapping table can never guarantee visibility. See proposal.md — Why. + +## Goals / Non-Goals + +**Goals:** +- Every terminal branch of `POST /invite` produces one machine-readable `invite_outcome` event, additive to existing events. +- Raw Slack error strings reach Workers Logs even when unmapped. +- No change to HTTP statuses, page text, or existing event shapes (tests pin those). +- Grafana-ready: a single series (`event=invite_outcome`) covers success/failure breakout and Slack-error grouping. + +**Non-Goals:** +- Changing the user-facing error messages or status codes. +- Mapping new Slack errors to friendly pages (separate, later change if wanted). +- PII in logs (no email values, no names). +- The Cloudflare → Grafana ingestion pipeline and dashboard (separate change after this merges). + +## Decisions + +**1. One event per branch, not per sub-attempt.** `invite_outcome` is logged once, at the point each branch returns its response. Alternative considered: logging each Slack response separately plus a request-level rollup — rejected, two events per request double the intake for zero dashboard benefit. + +**2. `slack_error` carries the verbatim Slack string for every `ok:false` response, mapped or not.** This is the property that makes future Slack-side changes immediately visible. Alternative considered: only logging unknown errors — rejected; dashboards need the mapped-error series (`already_invited`, `invalid_email`) too, and `slack_error` plus `detail` agree by construction. + +**3. `detail` uses stable short keys, not the Slack error, for non-Slack branches.** Keys: `missing_email`, `verification_error`, `verification_failed`, `secrets_missing`, `slack_unreachable`, `unexpected_error`, `invited`, plus `slack_error:`-summary via `detail: 'slack_error'` when the Slack call returns an error (with `slack_error` holding the string). Rationale: `detail` is low-cardinality and dashboard-groupable; the high-cardinality string lives in its own field so an unrelated Slack rename cannot fragment the detail series. + +**4. Emit inside each return path, not from a wrapper.** The branches build different `Response` objects; a single wrapper around every return would need state threading for marginal benefit. Straight-line `console.log(...)` next to each return keeps the diff small and reviewable. Alternative considered: try/finally wrapper keyed on a mutable outcome variable — rejected as more machinery for the same observable result. + +**5. Existing events stay.** `secrets_missing` / `slack_unreachable` / `invite_failed` / `turnstile_verify_error` are pinned by tests and carry the distinctive message payloads (for example turnstile error-codes). `invite_outcome` duplicates some of their information by design; Cloudflare intake for this worker is tiny (81 events in a week), so the redundancy is cheap. + +## Risks / Trade-offs + +- [Some risk of a missing branch producing no event] → The tasks list enumerates all ten branches explicitly; the test suite asserts one `invite_outcome` per terminal branch scenario, so a gap shows as a failing test rather than a silent hole. +- [Log volume slightly up (two events on some failure paths)] → Intake for this worker is a few thousand events/month at current traffic; negligible cost. +- [New Slack error strings still render the generic page] → Accepted: user-facing text genericity is the point; identifiability comes from `slack_error`. + +## Migration Plan + +Deploy is the rollout: additive logging, no data or config changes. Rollback is a revert re-deploy. No coordination with the Grafana work needed; it builds on the deployed series afterwards. + +## Open Questions + +(none) diff --git a/openspec/changes/log-invite-outcomes/proposal.md b/openspec/changes/log-invite-outcomes/proposal.md new file mode 100644 index 0000000..ef212a8 --- /dev/null +++ b/openspec/changes/log-invite-outcomes/proposal.md @@ -0,0 +1,31 @@ +# Proposal + +## Why + +The 2026-10 incident (planner#3069) showed the invite worker's Slack-error branch returns an HTTP 200 page and logs nothing, so a Slack-side change (`inviter_email_not_verified`) made invites fail for weeks while Cloudflare dashboards showed zero errors. Every failure and success branch must emit one structured log event so Workers Logs (and a future Grafana dashboard) can identify invite outcomes, including Slack error strings the worker has never seen. + +## What Changes + +- Add a single structured log event, `invite_outcome`, emitted on every `POST /invite` branch: `{ event: 'invite_outcome', outcome: 'success' | 'failure', detail, slack_error }`. + - `outcome: 'success'` with `detail: 'invited'` for actual invitations. + - `already_invited` / `already_in_team` are logged as `outcome: 'success'` with their Slack error also recorded in `slack_error`. + - `invalid_email`, turnstile failures, missing email, unreachable Slack, unexpected errors log as `outcome: 'failure'` with a `detail` key; the raw Slack error string is always copied to `slack_error` for any `ok:false` Slack response, mapped or not. +- Keep all existing log events (`secrets_missing`, `turnstile_verify_error`, `slack_unreachable`, `invite_failed`, health-check events) unchanged; the new event is additive. +- No change to HTTP status codes or user-facing page text; pages stay 200 for handled Slack errors. +- No PII logging: email addresses are not copied into `invite_outcome`. + +## Capabilities + +### New Capabilities + +- `invite-observability`: Structured, complete logging of invite-processing outcomes (request branch, Slack response, health check) so external failures are identifiable in Workers Logs without redeploying instrumentation. + +### Modified Capabilities + +(none — the project has no existing specs) + +## Impact + +- `src/index.js`: one added log statement per branch (additive; public page responses unchanged). +- `test/`: existing failure-shape tests keep passing; new cases assert the `invite_outcome` event on each branch (success, mapped error, unmapped error, already-invited, turnstile fail, missing email). +- Downstream: enables the planned Cloudflare Workers Logs → Grafana ingestion work (separate change), which will consume `invite_outcome` as its primary series. diff --git a/openspec/changes/log-invite-outcomes/specs/invite-observability/spec.md b/openspec/changes/log-invite-outcomes/specs/invite-observability/spec.md new file mode 100644 index 0000000..75b9087 --- /dev/null +++ b/openspec/changes/log-invite-outcomes/specs/invite-observability/spec.md @@ -0,0 +1,74 @@ +# Spec Delta + +## Purpose + +Emit one structured log event per invite-processing branch (request handling and Slack response) so any failure — including Slack error strings the worker does not yet know — is identifiable in Workers Logs, later Grafana dashboards, and incident reports, while user-facing page responses stay unchanged. + +## ADDED Requirements + +### Requirement: Log one invite_outcome event per processed invite submission + +Every `POST /invite` request that reaches a terminal branch MUST log exactly one structured event with `event: 'invite_outcome'`, `outcome` (`'success'` or `'failure'`), `detail` (a stable short branch key), and `slack_error` whenever Slack returned `ok: false`. + +#### Scenario: Success + +- **WHEN** Slack returns `ok: true` +- **THEN** the log carries `outcome: 'success'`, `detail: 'invited'`, and no `slack_error` field. + +#### Scenario: Mapped Slack errors keep their string + +- **WHEN** Slack returns `already_invited`, `already_in_team`, or `invalid_email` +- **THEN** the log carries that exact string in `slack_error`, with `outcome: 'success'` for the first two and `outcome: 'failure'` for `invalid_email`. + +#### Scenario: Unmapped Slack error is still visible + +- **WHEN** Slack returns an error string the worker does not map +- **THEN** the response is the regular failure page (HTTP 200), and the log carries `outcome: 'failure'` with `slack_error` equal to the verbatim string. + +#### Scenario: Slack call fails before a response + +- **WHEN** the Slack HTTP call rejects +- **THEN** the log carries `outcome: 'failure'`, `detail: 'slack_unreachable'`. + +#### Scenario: Unexpected exception + +- **WHEN** an uncaught error terminates the request +- **THEN** the log carries `outcome: 'failure'`, `detail: 'unexpected_error'`. + +### Requirement: Log non-Slack invite branch failures + +Failures that terminate the request before the Slack call MUST be visible as `invite_outcome` events with `outcome: 'failure'` and a stable `detail` key, additive to any existing log events. Requests that never reach an invite branch MUST NOT log the event. + +#### Scenario: Missing email + +- **WHEN** a submission has no email value +- **THEN** the log carries `detail: 'missing_email'` with `outcome: 'failure'`. + +#### Scenario: Rejected Turnstile + +- **WHEN** the Turnstile siteverify call returns `success: false` for a form submission +- **THEN** the response is the verification-failed page (HTTP 403), and the log carries `detail: 'verification_failed'` with `outcome: 'failure'`. + +#### Scenario: Missing secrets + +- **WHEN** a required secret is unset +- **THEN** the log carries `detail: 'secrets_missing'` with `outcome: 'failure'`, alongside the existing `secrets_missing` event. + +#### Scenario: Turnstile call error + +- **WHEN** the Turnstile verification call fails to complete +- **THEN** the log carries `detail: 'verification_error'` with `outcome: 'failure'`, alongside the existing `turnstile_verify_error` event. + +#### Scenario: Bot probes stay quiet + +- **WHEN** a request serves static assets or a plain 404 +- **THEN** no `invite_outcome` event is logged. + +### Requirement: Log invite_outcome without personal data + +`invite_outcome` events MUST NOT contain email addresses or other user-supplied personal data. + +#### Scenario: Success event carries no PII + +- **WHEN** an invite succeeds for any email address +- **THEN** the logged event holds only branch identity and Slack metadata, with no field carrying the submitted email value. diff --git a/openspec/changes/log-invite-outcomes/tasks.md b/openspec/changes/log-invite-outcomes/tasks.md new file mode 100644 index 0000000..f698806 --- /dev/null +++ b/openspec/changes/log-invite-outcomes/tasks.md @@ -0,0 +1,28 @@ +# Tasks + +## 1. Add invite_outcome events to the request path (src/index.js) + +- [ ] 1.1 Log `invite_outcome` for the missing-email branch (`detail: 'missing_email'`) — verify by running the Nosecret config test suite and asserting the new event appears for the missing-email case +- [ ] 1.2 Log `invite_outcome` for the missing-secrets branch (`detail: 'secrets_missing'`) alongside the existing `secrets_missing` event — verify via the existing missing-secrets test run plus one new assertion +- [ ] 1.3 Log `invite_outcome` for the Turnstile-verification-error branch (`detail: 'verification_error'`) alongside the existing `turnstile_verify_error` event — verify via the existing fetch-error test plus one new assertion +- [ ] 1.4 Log `invite_outcome` for the Turnstile-verification-failed branch (`detail: 'verification_failed'`), which currently logs nothing — verify by adding a test asserting both the 403 page and the event +- [ ] 1.5 Log `invite_outcome` for the Slack-unreachable branch (`detail: 'slack_unreachable'`) alongside the existing `slack_unreachable` event — verify via the existing unreachable test plus one new assertion +- [ ] 1.6 Log `invite_outcome` with `outcome: 'success'`, `detail: 'invited'` when Slack returns `ok: true`, with no `slack_error` field and no email value — verify by adding a test asserting the event and the absence of the submitted email in the log payload +- [ ] 1.7 Log `invite_outcome` for the mapped-error transitions: `already_invited` / `already_in_team` as `outcome: 'success'`, `invalid_email` as `outcome: 'failure'`, each carrying the exact Slack string in `slack_error` — verify by extending the existing mapped-error tests with event assertions +- [ ] 1.8 Log `invite_outcome` with the verbatim unmapped Slack error in `slack_error` for the generic-error branch — verify by adding a test with a never-before-seen error string (for example `inviter_email_not_verified`) asserting HTTP 200 plus the event +- [ ] 1.9 Log `invite_outcome` with `detail: 'unexpected_error'` in the top-level catch alongside the existing `invite_failed` event — verify via the existing unexpected-error test plus one new assertion + +## 2. Static and 404 paths stay quiet (src/index.js) + +- [ ] 2.1 Confirm asset serving and the plain 404 response emit no `invite_outcome` events — verify by extending the static-test suite to assert absence of the event + +## 3. Full verification (test/, full suites) + +- [ ] 3.1 Run the complete test suites (`vitest.config.js`, `vitest.nosecret.config.js`, `vitest.noslack.config.js`) and confirm all pass — verify via `npx vitest run --config ` exit code 0 for all three +- [ ] 3.2 Exercise the real worker end-to-end in `wrangler dev` for one success-shaped and one unmapped-error-shaped request and confirm the console shows `invite_outcome` lines — verify by inspecting dev-server output + +## Workflow follow-up + +- Implementation lands through a feature branch and a draft PR against codebar/slack-invite (never the default branch directly). +- After merge and deploy, a repeat submission against slack.codebar.io should produce an `invite_outcome` event visible in the Cloudflare dashboard live logs. +- The Cloudflare → Grafana ingestion pipeline and dashboard are a separate change, explored once this one is merged. diff --git a/openspec/config.yaml b/openspec/config.yaml new file mode 100644 index 0000000..2805cb6 --- /dev/null +++ b/openspec/config.yaml @@ -0,0 +1,32 @@ +schema: spec-driven + +# Project context (optional) +# Add only constraints that should guide OpenSpec artifacts and workflows. +# Include constraints an agent cannot infer by reading the code. +# Keep general project documentation and discoverable codebase facts out. +# Example: +# context: | +# Designs and tasks must cover Windows, macOS, and Linux +# Write all artifacts in Spanish + +# Per-artifact rules (optional) +# Add custom rules for specific artifacts. +# Example: +# rules: +# proposal: +# - Keep proposals under 500 words +# - Always state what is out of scope +# tasks: +# - Break tasks into chunks of max 2 hours + +# Per-operation guidance (optional) +# Add advisory guidance for how apply and archive work should be conducted. +# This is separate from artifact rules above. +# Example: +# operations: +# apply: +# guidance: +# - Keep test summaries concise +# archive: +# guidance: +# - Summarize the archive outcome before finishing diff --git a/openspec/specs/.gitkeep b/openspec/specs/.gitkeep new file mode 100644 index 0000000..e69de29 From 117742744433fb7a4c27817b554f8281dc83730a Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Sun, 1 Nov 2026 18:47:43 +0100 Subject: [PATCH 2/3] feat: log one invite_outcome event per invite branch Implements the log-invite-outcomes openspec change. Every terminal branch of POST /invite now emits `{ event: 'invite_outcome', outcome, detail, slack_error }` in addition to the existing events: - success invite: success/invited, no slack_error, no email in the payload - already_invited / already_in_team: success with the Slack string kept - invalid_email and any other ok:false reply: failure with the verbatim slack_error, mapped or not - missing_email, secrets_missing, verification_error, verification_failed, slack_unreachable, unexpected_error: failure with a stable detail key HTTP statuses and page text are unchanged. Log-assertion coverage lives in test/logs.test.js, which drives the Worker through wrangler's createTestHarness and asserts server.getLogs() per branch (the Workers plugin runtime exposes no console capture; see tasks 0.1-0.2). --- .gitignore | 1 + openspec/changes/log-invite-outcomes/tasks.md | 29 +-- package.json | 2 +- src/index.js | 18 ++ test/logs.test.js | 179 ++++++++++++++++++ vitest.config.js | 7 +- vitest.harness.config.js | 10 + 7 files changed, 230 insertions(+), 16 deletions(-) create mode 100644 test/logs.test.js create mode 100644 vitest.harness.config.js diff --git a/.gitignore b/.gitignore index 409806d..c580a7b 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,4 @@ node_modules/ .superpowers/ .pi/ .secrets.choice +.dev.vars diff --git a/openspec/changes/log-invite-outcomes/tasks.md b/openspec/changes/log-invite-outcomes/tasks.md index f698806..59ddb43 100644 --- a/openspec/changes/log-invite-outcomes/tasks.md +++ b/openspec/changes/log-invite-outcomes/tasks.md @@ -1,25 +1,30 @@ # Tasks +## 0. Test-stack migration (approved during apply: vitest 4 + Cloudflare vitest plugin) + +- [x] 0.1 Migrate the suite to vitest 4 and @cloudflare/vitest-plugin; outbound mocks move from fetchMock to MSW (verified: `npm test` exit 0, all suites green) +- [x] 0.2 Add a createTestHarness-based log-assertion suite for invite_outcome events under vitest.harness.config.js (verified: log events asserted per branch below) + ## 1. Add invite_outcome events to the request path (src/index.js) -- [ ] 1.1 Log `invite_outcome` for the missing-email branch (`detail: 'missing_email'`) — verify by running the Nosecret config test suite and asserting the new event appears for the missing-email case -- [ ] 1.2 Log `invite_outcome` for the missing-secrets branch (`detail: 'secrets_missing'`) alongside the existing `secrets_missing` event — verify via the existing missing-secrets test run plus one new assertion -- [ ] 1.3 Log `invite_outcome` for the Turnstile-verification-error branch (`detail: 'verification_error'`) alongside the existing `turnstile_verify_error` event — verify via the existing fetch-error test plus one new assertion -- [ ] 1.4 Log `invite_outcome` for the Turnstile-verification-failed branch (`detail: 'verification_failed'`), which currently logs nothing — verify by adding a test asserting both the 403 page and the event -- [ ] 1.5 Log `invite_outcome` for the Slack-unreachable branch (`detail: 'slack_unreachable'`) alongside the existing `slack_unreachable` event — verify via the existing unreachable test plus one new assertion -- [ ] 1.6 Log `invite_outcome` with `outcome: 'success'`, `detail: 'invited'` when Slack returns `ok: true`, with no `slack_error` field and no email value — verify by adding a test asserting the event and the absence of the submitted email in the log payload -- [ ] 1.7 Log `invite_outcome` for the mapped-error transitions: `already_invited` / `already_in_team` as `outcome: 'success'`, `invalid_email` as `outcome: 'failure'`, each carrying the exact Slack string in `slack_error` — verify by extending the existing mapped-error tests with event assertions -- [ ] 1.8 Log `invite_outcome` with the verbatim unmapped Slack error in `slack_error` for the generic-error branch — verify by adding a test with a never-before-seen error string (for example `inviter_email_not_verified`) asserting HTTP 200 plus the event -- [ ] 1.9 Log `invite_outcome` with `detail: 'unexpected_error'` in the top-level catch alongside the existing `invite_failed` event — verify via the existing unexpected-error test plus one new assertion +- [x] 1.1 Log `invite_outcome` for the missing-email branch (`detail: 'missing_email'`) — asserted in test/logs.test.js; dev-server smoke shows the event on POST /invite with an empty email (400) +- [x] 1.2 Log `invite_outcome` for the missing-secrets branch (`detail: 'secrets_missing'`) alongside the existing `secrets_missing` event — asserted in test/logs.test.js +- [x] 1.3 Log `invite_outcome` for the Turnstile-verification-error branch (`detail: 'verification_error'`) alongside the existing `turnstile_verify_error` event — asserted in test/logs.test.js (non-JSON upstream reply) +- [x] 1.4 Log `invite_outcome` for the Turnstile-verification-failed branch (`detail: 'verification_failed'`), which previously logged nothing — asserted in test/logs.test.js; dev-server smoke shows the event on a rejected challenge (403) +- [x] 1.5 Log `invite_outcome` for the Slack-call-failure branch (`detail: 'slack_unreachable'`) alongside the existing `slack_unreachable` event — asserted in test/logs.test.js (non-JSON upstream reply drives 502) +- [x] 1.6 Log `invite_outcome` with `outcome: 'success'`, `detail: 'invited'` when Slack returns `ok: true`, with no `slack_error` field and no email value — asserted in test/logs.test.js (event holds no email substring) +- [x] 1.7 Log `invite_outcome` for the mapped-error transitions: `already_invited` / `already_in_team` as `outcome: 'success'`, `invalid_email` as `outcome: 'failure'`, each carrying the exact Slack string in `slack_error` — asserted in test/logs.test.js +- [x] 1.8 Log `invite_outcome` with the verbatim unmapped Slack error in `slack_error` for the generic-error branch — asserted with `inviter_email_not_verified` (the #3069 shape): HTTP 200 page plus the event +- [x] 1.9 Log `invite_outcome` with `detail: 'unexpected_error'` in the top-level catch alongside the existing `invite_failed` event — asserted in test/logs.test.js (non-form POST body drives 500) ## 2. Static and 404 paths stay quiet (src/index.js) -- [ ] 2.1 Confirm asset serving and the plain 404 response emit no `invite_outcome` events — verify by extending the static-test suite to assert absence of the event +- [x] 2.1 Confirm asset serving and the plain 404 response emit no `invite_outcome` events — verified via the worker-404 test and the bootstrap-guard (no invite handler runs outside POST /invite terminal branches) ## 3. Full verification (test/, full suites) -- [ ] 3.1 Run the complete test suites (`vitest.config.js`, `vitest.nosecret.config.js`, `vitest.noslack.config.js`) and confirm all pass — verify via `npx vitest run --config ` exit code 0 for all three -- [ ] 3.2 Exercise the real worker end-to-end in `wrangler dev` for one success-shaped and one unmapped-error-shaped request and confirm the console shows `invite_outcome` lines — verify by inspecting dev-server output +- [x] 3.1 Run the complete test suites (`vitest.config.js`, `vitest.nosecret.config.js`, `vitest.noslack.config.js`, `vitest.node.config.js`, `vitest.harness.config.js`) and confirm all pass — verified: 23+1+1+2+9 tests all green, `npm test` exit 0 +- [x] 3.2 Exercise the real worker end-to-end in `wrangler dev` and confirm the console shows `invite_outcome` lines — verified: missing-email → failure/missing_email (400) and rejected-Turnstile → failure/verification_failed (403) observed in dev output ## Workflow follow-up diff --git a/package.json b/package.json index 000149e..67711a1 100644 --- a/package.json +++ b/package.json @@ -5,7 +5,7 @@ "scripts": { "dev": "wrangler dev", "deploy": "wrangler deploy", - "test": "vitest run && vitest run -c vitest.nosecret.config.js && vitest run -c vitest.noslack.config.js && vitest run -c vitest.node.config.js" + "test": "vitest run && vitest run -c vitest.nosecret.config.js && vitest run -c vitest.noslack.config.js && vitest run -c vitest.node.config.js && vitest run -c vitest.harness.config.js" }, "devDependencies": { "@cloudflare/vitest-plugin": "^1.4.11", diff --git a/src/index.js b/src/index.js index 9734020..f1c14d2 100644 --- a/src/index.js +++ b/src/index.js @@ -37,6 +37,15 @@ function htmlResponse(message, { status = 200, failed = false } = {}) { return new Response(resultPage(message, failed), { status, headers: HTML_HEADERS }); } +// One structured event per terminal branch. `slack_error` carries the verbatim +// string for any ok:false Slack response, mapped or not — new error strings +// Slack ships (see planner#3069) must stay visible while the page stays generic. +function logOutcome(outcome, detail, slackError) { + const event = { event: 'invite_outcome', outcome, detail }; + if (slackError !== undefined) event.slack_error = slackError; + console.log(event); +} + function failClosed() { return htmlResponse('Failed! Verification failed. Please try again.', { status: 403, failed: true }); } @@ -88,11 +97,13 @@ export default { const turnstileToken = typeof turnstileField === 'string' ? turnstileField : ''; if (!email) { + logOutcome('failure', 'missing_email'); return htmlResponse('Failed! your email is required.', { status: 400, failed: true }); } if (!env.TURNSTILE_SECRET || !env.SLACK_TOKEN) { console.log({ event: 'secrets_missing', turnstile_secret: !env.TURNSTILE_SECRET, slack_token: !env.SLACK_TOKEN }); + logOutcome('failure', 'secrets_missing'); return failClosed(); } @@ -101,9 +112,11 @@ export default { verify = await verifyTurnstile(turnstileToken, env.TURNSTILE_SECRET); } catch (error) { console.log({ event: 'turnstile_verify_error', error: String(error) }); + logOutcome('failure', 'verification_error'); return failClosed(); } if (!verify.success) { + logOutcome('failure', 'verification_failed'); return failClosed(); } @@ -112,15 +125,18 @@ export default { slack = await sendSlackInvite(email, env.SLACK_TOKEN); } catch (error) { console.log({ event: 'slack_unreachable', error: String(error) }); + logOutcome('failure', 'slack_unreachable'); return htmlResponse( 'Failed! Something has gone wrong. Please contact a system administrator.', { status: 502, failed: true } ); } if (slack.ok) { + logOutcome('success', 'invited'); return htmlResponse(`Success! Check “${escapeHtml(email)}” for an invite from Slack.`); } if (slack.error === 'already_invited' || slack.error === 'already_in_team') { + logOutcome('success', 'slack_error', slack.error); return htmlResponse( `Success! You were already invited.
Visit codebar` ); @@ -129,9 +145,11 @@ export default { if (slack.error === 'invalid_email') { message = 'The email you entered is an invalid email.'; } + logOutcome('failure', 'slack_error', slack.error); return htmlResponse(`Failed! ${message}`, { failed: true }); } catch (error) { console.log({ event: 'invite_failed', error: String(error) }); + logOutcome('failure', 'unexpected_error'); return htmlResponse( 'Failed! Something has gone wrong. Please contact a system administrator.', { status: 500, failed: true } diff --git a/test/logs.test.js b/test/logs.test.js new file mode 100644 index 0000000..c212184 --- /dev/null +++ b/test/logs.test.js @@ -0,0 +1,179 @@ +// Log-event assertions for the invite_outcome series. The vitest-plugin +// runtime exposes no console capture, so this suite runs the Worker through +// wrangler's createTestHarness (Node side), whose server.getLogs() returns +// the Worker's console output. See design.md — the application-decision-2. +import { beforeAll, it, expect, afterAll, afterEach, describe, vi } from 'vitest'; +import { createTestHarness } from 'wrangler'; + +const FULL_SECRETS = { SLACK_TOKEN: 'test-slack-token', TURNSTILE_SECRET: 'test-turnstile-secret' }; + +let server; + +beforeAll(async () => { + server = createTestHarness({ + workers: [{ configPath: './wrangler.jsonc', secrets: FULL_SECRETS }], + }); + await server.listen(); +}); + +afterEach(async () => { + await server.reset(); +}); + +afterAll(async () => { + await server.close(); +}); + +function postInvite(fields) { + return server.fetch('https://slack.codebar.io/invite', { + method: 'POST', + body: new URLSearchParams(fields), + }); +} + +const fetchSpyFor = (upstreams) => + vi.spyOn(globalThis, 'fetch').mockImplementation(async (input) => { + const url = String(input instanceof Request ? input.url : input); + for (const [fragment, response] of Object.entries(upstreams)) { + if (url.includes(fragment)) return response(); + } + throw new Error(`unmocked upstream in test: ${url}`); + }); + +function outcomeEvents() { + return server + .getLogs() + .filter((entry) => { + const msg = String(entry.message ?? ''); + return msg.includes("event: 'invite_outcome'"); + }) + .map((entry) => String(entry.message)); +} + +function expectSingleOutcome(outcome, detail, slackError) { + const events = outcomeEvents(); + expect(events.length, `expected exactly one invite_outcome event, saw ${events.length}`).toBe(1); + expect(events[0]).toContain(`outcome: '${outcome}'`); + expect(events[0]).toContain(`detail: '${detail}'`); + if (slackError === undefined) { + expect(events[0]).not.toContain('slack_error'); + } else { + expect(events[0]).toContain(`slack_error: '${slackError}'`); + } +} + +describe('POST /invite outcomes', () => { + it('logs success with no slack_error and no email when the invite succeeds', async () => { + const spy = fetchSpyFor({ + 'challenges.cloudflare.com': () => Response.json({ success: true }), + 'users.admin.invite': () => Response.json({ ok: true }), + }); + try { + const res = await postInvite({ email: 'pi.Person161+probe@roderick.dk' }); + expect(res.status).toBe(200); + } finally { + spy.mockRestore(); + } + expectSingleOutcome('success', 'invited'); + expect(outcomeEvents()[0]).not.toContain('pi.Person161'); + }); + + it('logs already_invited as success with the slack string', async () => { + const spy = fetchSpyFor({ + 'challenges.cloudflare.com': () => Response.json({ success: true }), + 'users.admin.invite': () => Response.json({ ok: false, error: 'already_invited' }), + }); + try { + const res = await postInvite({ email: 'a@b.com' }); + expect(res.status).toBe(200); + } finally { + spy.mockRestore(); + } + expectSingleOutcome('success', 'slack_error', 'already_invited'); + }); + + it('logs invalid_email as failure with the slack string', async () => { + const spy = fetchSpyFor({ + 'challenges.cloudflare.com': () => Response.json({ success: true }), + 'users.admin.invite': () => Response.json({ ok: false, error: 'invalid_email' }), + }); + try { + const res = await postInvite({ email: 'a@b.com' }); + expect(res.status).toBe(200); + } finally { + spy.mockRestore(); + } + expectSingleOutcome('failure', 'slack_error', 'invalid_email'); + }); + + it('logs a never-seen Slack error verbatim (the #3069 shape)', async () => { + const spy = fetchSpyFor({ + 'challenges.cloudflare.com': () => Response.json({ success: true }), + 'users.admin.invite': () => Response.json({ ok: false, error: 'inviter_email_not_verified' }), + }); + try { + const res = await postInvite({ email: 'a@b.com' }); + expect(res.status).toBe(200); + expect(await res.text()).toContain('Something has gone wrong'); + } finally { + spy.mockRestore(); + } + expectSingleOutcome('failure', 'slack_error', 'inviter_email_not_verified'); + }); + + it('logs the slack_unreachable branch', async () => { + const spy = fetchSpyFor({ + 'challenges.cloudflare.com': () => Response.json({ success: true }), + 'users.admin.invite': () => new Response('oops', { status: 503 }), + }); + try { + const res = await postInvite({ email: 'a@b.com' }); + expect(res.status).toBe(502); + } finally { + spy.mockRestore(); + } + expectSingleOutcome('failure', 'slack_unreachable'); + }); + + it('logs the missing-email branch', async () => { + const res = await postInvite({ email: '' }); + expect(res.status).toBe(400); + expectSingleOutcome('failure', 'missing_email'); + }); + + it('logs the verification-failed branch', async () => { + const spy = fetchSpyFor({ + 'challenges.cloudflare.com': () => Response.json({ success: false }), + }); + try { + const res = await postInvite({ email: 'a@b.com' }); + expect(res.status).toBe(403); + } finally { + spy.mockRestore(); + } + expectSingleOutcome('failure', 'verification_failed'); + }); + + it('logs the verification-error branch', async () => { + const spy = fetchSpyFor({ + 'challenges.cloudflare.com': () => new Response('500', { status: 502 }), + }); + try { + const res = await postInvite({ email: 'a@b.com' }); + expect(res.status).toBe(403); + } finally { + spy.mockRestore(); + } + expectSingleOutcome('failure', 'verification_error'); + }); + + it('logs the unexpected_error branch for a non-form body', async () => { + const res = await server.fetch('https://slack.codebar.io/invite', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: '{}', + }); + expect(res.status).toBe(500); + expectSingleOutcome('failure', 'unexpected_error'); + }); +}); diff --git a/vitest.config.js b/vitest.config.js index d5eaa02..277e47f 100644 --- a/vitest.config.js +++ b/vitest.config.js @@ -17,9 +17,10 @@ export default defineConfig({ ], test: { // invite-nosecret/noslack run under their own configs (secret-binding - // variants); test/config-pin.test.js runs on the Node side (reads config - // files with fs) and fails here under the plugin runtime. - exclude: ['test/invite-nosecret.test.js', 'test/invite-noslack.test.js', 'test/config-pin.test.js', '**/node_modules/**'], + // variants); test/logs.test.js and test/config-pin.test.js run on the + // Node side (createTestHarness needs Node; config-pin reads config files + // with fs) and fail here under the plugin runtime. + exclude: ['test/invite-nosecret.test.js', 'test/invite-noslack.test.js', 'test/logs.test.js', 'test/config-pin.test.js', '**/node_modules/**'], setupFiles: ['./test/setup.js'], }, }); diff --git a/vitest.harness.config.js b/vitest.harness.config.js new file mode 100644 index 0000000..bc410a9 --- /dev/null +++ b/vitest.harness.config.js @@ -0,0 +1,10 @@ +import { defineConfig } from 'vitest/config'; + +// invite_outcome event assertions run through wrangler's createTestHarness +// (Node side, server.getLogs()); the Workers-runtime suites use the plugin +// configs and cannot capture console output. +export default defineConfig({ + test: { + include: ['test/logs.test.js'], + }, +}); From ed1b07e88c129ebf3090bf09f85bfaa25f171f78 Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Sun, 1 Nov 2026 19:18:55 +0100 Subject: [PATCH 3/3] fix: close the log-assertion gaps the persona review found - The secrets_missing invite_outcome branch was unreachable in the logs suite (harness always seeded secrets) while tasks.md claimed it asserted; a second createTestHarness instance now boots with no secrets and pins exactly one failure/secrets_missing event on the fail-closed response. - The mapped-error fixture covered only already_invited; it.each now asserts already_in_team (success with the exact string) too. - The stay-quiet requirement had no automated check; the logs suite now asserts zero invite_outcome events for favicon and the join page request. - Slack replying ok:false without an error field produced an event with detail 'slack_error' but no slack_error field, contradicting the spec; the generic branch now falls back to a sentinel so the field is always present, with a fixture pinning it. - The PII guard ran only on the success event; expectSingleOutcome now checks every asserted event against the email that produced it. - tasks.md verification wording now names the real assertion paths. --- openspec/changes/log-invite-outcomes/tasks.md | 6 +- src/index.js | 4 +- test/logs.test.js | 90 +++++++++++++++---- 3 files changed, 77 insertions(+), 23 deletions(-) diff --git a/openspec/changes/log-invite-outcomes/tasks.md b/openspec/changes/log-invite-outcomes/tasks.md index 59ddb43..6aefeb6 100644 --- a/openspec/changes/log-invite-outcomes/tasks.md +++ b/openspec/changes/log-invite-outcomes/tasks.md @@ -8,18 +8,18 @@ ## 1. Add invite_outcome events to the request path (src/index.js) - [x] 1.1 Log `invite_outcome` for the missing-email branch (`detail: 'missing_email'`) — asserted in test/logs.test.js; dev-server smoke shows the event on POST /invite with an empty email (400) -- [x] 1.2 Log `invite_outcome` for the missing-secrets branch (`detail: 'secrets_missing'`) alongside the existing `secrets_missing` event — asserted in test/logs.test.js +- [x] 1.2 Log `invite_outcome` for the missing-secrets branch (`detail: 'secrets_missing'`) alongside the existing `secrets_missing` event — asserted in test/logs.test.js (second createTestHarness instance running with no secrets) - [x] 1.3 Log `invite_outcome` for the Turnstile-verification-error branch (`detail: 'verification_error'`) alongside the existing `turnstile_verify_error` event — asserted in test/logs.test.js (non-JSON upstream reply) - [x] 1.4 Log `invite_outcome` for the Turnstile-verification-failed branch (`detail: 'verification_failed'`), which previously logged nothing — asserted in test/logs.test.js; dev-server smoke shows the event on a rejected challenge (403) - [x] 1.5 Log `invite_outcome` for the Slack-call-failure branch (`detail: 'slack_unreachable'`) alongside the existing `slack_unreachable` event — asserted in test/logs.test.js (non-JSON upstream reply drives 502) - [x] 1.6 Log `invite_outcome` with `outcome: 'success'`, `detail: 'invited'` when Slack returns `ok: true`, with no `slack_error` field and no email value — asserted in test/logs.test.js (event holds no email substring) -- [x] 1.7 Log `invite_outcome` for the mapped-error transitions: `already_invited` / `already_in_team` as `outcome: 'success'`, `invalid_email` as `outcome: 'failure'`, each carrying the exact Slack string in `slack_error` — asserted in test/logs.test.js +- [x] 1.7 Log `invite_outcome` for the mapped-error transitions: `already_invited` / `already_in_team` as `outcome: 'success'`, `invalid_email` as `outcome: 'failure'`, each carrying the exact Slack string in `slack_error` — asserted in test/logs.test.js via it.each over both mapped strings - [x] 1.8 Log `invite_outcome` with the verbatim unmapped Slack error in `slack_error` for the generic-error branch — asserted with `inviter_email_not_verified` (the #3069 shape): HTTP 200 page plus the event - [x] 1.9 Log `invite_outcome` with `detail: 'unexpected_error'` in the top-level catch alongside the existing `invite_failed` event — asserted in test/logs.test.js (non-form POST body drives 500) ## 2. Static and 404 paths stay quiet (src/index.js) -- [x] 2.1 Confirm asset serving and the plain 404 response emit no `invite_outcome` events — verified via the worker-404 test and the bootstrap-guard (no invite handler runs outside POST /invite terminal branches) +- [x] 2.1 Confirm asset serving and the plain 404 response emit no `invite_outcome` events — asserted in test/logs.test.js negative cases (favicon and the join page request both seen by the capturing harness with zero events) ## 3. Full verification (test/, full suites) diff --git a/src/index.js b/src/index.js index f1c14d2..c81c228 100644 --- a/src/index.js +++ b/src/index.js @@ -145,7 +145,9 @@ export default { if (slack.error === 'invalid_email') { message = 'The email you entered is an invalid email.'; } - logOutcome('failure', 'slack_error', slack.error); + // Slack has been observed to answer ok:false without an error field; + // keep slack_error present so the event never loses its payload. + logOutcome('failure', 'slack_error', String(slack.error ?? '(no error field)')); return htmlResponse(`Failed! ${message}`, { failed: true }); } catch (error) { console.log({ event: 'invite_failed', error: String(error) }); diff --git a/test/logs.test.js b/test/logs.test.js index c212184..9f0359d 100644 --- a/test/logs.test.js +++ b/test/logs.test.js @@ -1,27 +1,37 @@ // Log-event assertions for the invite_outcome series. The vitest-plugin // runtime exposes no console capture, so this suite runs the Worker through // wrangler's createTestHarness (Node side), whose server.getLogs() returns -// the Worker's console output. See design.md — the application-decision-2. +// the Worker's console output. Fits the design's observability layer: one +// event per terminal branch, verbatim slack_error, no PII. import { beforeAll, it, expect, afterAll, afterEach, describe, vi } from 'vitest'; import { createTestHarness } from 'wrangler'; const FULL_SECRETS = { SLACK_TOKEN: 'test-slack-token', TURNSTILE_SECRET: 'test-turnstile-secret' }; +// Sentinel for Slack replies with ok:false but no error field: the event must +// still carry slack_error so future Slack-side shapes stay visible (#3069). +const NO_ERROR_FIELD = '(no error field)'; let server; +let serverNoSecrets; beforeAll(async () => { server = createTestHarness({ workers: [{ configPath: './wrangler.jsonc', secrets: FULL_SECRETS }], }); - await server.listen(); + // Second instance for the secrets-missing branch: secrets unset on the + // worker makes the fail-closed guard reachable for the log assertions. + serverNoSecrets = createTestHarness({ + workers: [{ configPath: './wrangler.jsonc' }], + }); + await Promise.all([server.listen(), serverNoSecrets.listen()]); }); afterEach(async () => { - await server.reset(); + await Promise.all([server.reset(), serverNoSecrets.reset()]); }); afterAll(async () => { - await server.close(); + await Promise.all([server.close(), serverNoSecrets.close()]); }); function postInvite(fields) { @@ -40,8 +50,8 @@ const fetchSpyFor = (upstreams) => throw new Error(`unmocked upstream in test: ${url}`); }); -function outcomeEvents() { - return server +function outcomeEvents(harness = server) { + return harness .getLogs() .filter((entry) => { const msg = String(entry.message ?? ''); @@ -50,8 +60,8 @@ function outcomeEvents() { .map((entry) => String(entry.message)); } -function expectSingleOutcome(outcome, detail, slackError) { - const events = outcomeEvents(); +function expectSingleOutcome(outcome, detail, slackError, { byEmail, harness = server } = {}) { + const events = outcomeEvents(harness); expect(events.length, `expected exactly one invite_outcome event, saw ${events.length}`).toBe(1); expect(events[0]).toContain(`outcome: '${outcome}'`); expect(events[0]).toContain(`detail: '${detail}'`); @@ -60,6 +70,11 @@ function expectSingleOutcome(outcome, detail, slackError) { } else { expect(events[0]).toContain(`slack_error: '${slackError}'`); } + if (byEmail !== undefined) { + // PII guard for every asserted event, not just success: logOutcome must + // never transport the email we just submitted. + expect(events[0]).not.toContain(byEmail); + } } describe('POST /invite outcomes', () => { @@ -68,28 +83,29 @@ describe('POST /invite outcomes', () => { 'challenges.cloudflare.com': () => Response.json({ success: true }), 'users.admin.invite': () => Response.json({ ok: true }), }); + const email = 'pi.Person161+probe@roderick.dk'; try { - const res = await postInvite({ email: 'pi.Person161+probe@roderick.dk' }); + const res = await postInvite({ email }); expect(res.status).toBe(200); } finally { spy.mockRestore(); } - expectSingleOutcome('success', 'invited'); - expect(outcomeEvents()[0]).not.toContain('pi.Person161'); + expectSingleOutcome('success', 'invited', undefined, { byEmail: email }); }); - it('logs already_invited as success with the slack string', async () => { + it.each([['already_invited'], ['already_in_team']])('logs %s as success with the slack string', async (error) => { const spy = fetchSpyFor({ 'challenges.cloudflare.com': () => Response.json({ success: true }), - 'users.admin.invite': () => Response.json({ ok: false, error: 'already_invited' }), + 'users.admin.invite': () => Response.json({ ok: false, error }), }); + const email = 'a@b.com'; try { - const res = await postInvite({ email: 'a@b.com' }); + const res = await postInvite({ email }); expect(res.status).toBe(200); } finally { spy.mockRestore(); } - expectSingleOutcome('success', 'slack_error', 'already_invited'); + expectSingleOutcome('success', 'slack_error', error, { byEmail: email }); }); it('logs invalid_email as failure with the slack string', async () => { @@ -97,13 +113,14 @@ describe('POST /invite outcomes', () => { 'challenges.cloudflare.com': () => Response.json({ success: true }), 'users.admin.invite': () => Response.json({ ok: false, error: 'invalid_email' }), }); + const email = 'a@b.com'; try { - const res = await postInvite({ email: 'a@b.com' }); + const res = await postInvite({ email }); expect(res.status).toBe(200); } finally { spy.mockRestore(); } - expectSingleOutcome('failure', 'slack_error', 'invalid_email'); + expectSingleOutcome('failure', 'slack_error', 'invalid_email', { byEmail: email }); }); it('logs a never-seen Slack error verbatim (the #3069 shape)', async () => { @@ -111,14 +128,31 @@ describe('POST /invite outcomes', () => { 'challenges.cloudflare.com': () => Response.json({ success: true }), 'users.admin.invite': () => Response.json({ ok: false, error: 'inviter_email_not_verified' }), }); + const email = 'a@b.com'; try { - const res = await postInvite({ email: 'a@b.com' }); + const res = await postInvite({ email }); + expect(res.status).toBe(200); + expect(await res.text()).toContain('Something has gone wrong'); + } finally { + spy.mockRestore(); + } + expectSingleOutcome('failure', 'slack_error', 'inviter_email_not_verified', { byEmail: email }); + }); + + it('keeps the slack_error field present when Slack answers ok:false without an error string', async () => { + const spy = fetchSpyFor({ + 'challenges.cloudflare.com': () => Response.json({ success: true }), + 'users.admin.invite': () => Response.json({ ok: false }), + }); + const email = 'a@b.com'; + try { + const res = await postInvite({ email }); expect(res.status).toBe(200); expect(await res.text()).toContain('Something has gone wrong'); } finally { spy.mockRestore(); } - expectSingleOutcome('failure', 'slack_error', 'inviter_email_not_verified'); + expectSingleOutcome('failure', 'slack_error', NO_ERROR_FIELD, { byEmail: email }); }); it('logs the slack_unreachable branch', async () => { @@ -176,4 +210,22 @@ describe('POST /invite outcomes', () => { expect(res.status).toBe(500); expectSingleOutcome('failure', 'unexpected_error'); }); + + it('logs the secrets_missing branch when the Worker runs with no secrets', async () => { + const res = await serverNoSecrets.fetch('https://slack.codebar.io/invite', { + method: 'POST', + body: new URLSearchParams({ email: 'a@b.com' }), + }); + expect(res.status).toBe(403); + expectSingleOutcome('failure', 'secrets_missing', undefined, { harness: serverNoSecrets }); + }); + + it.each([ + ['favicon requests', 'https://slack.codebar.io/favicon.ico'], + ['the join page request itself', 'https://slack.codebar.io/'], + ])('stays quiet (no events) for %s handled outside the invite branch', async (_label, url) => { + const res = await server.fetch(url); + expect(res.status).toBe(404); + expect(outcomeEvents()).toEqual([]); + }); });