docs: propagate /v1 API versioning + fix stale counts and framing#162
Conversation
- README: /v1 note in API section (unversioned paths remain supported; infra endpoints unversioned), lead examples on /v1; Tests badge 200+ → 700+; testing section 82 → 700+ with resilient phrasing; Python SDK block 118 → 300+; roadmap notes Phases 13-17 built-but-parked - SETUP: /health example reports server build 1.0.0 (was 0.2.0) + checks field; /v1 note in §14; header disambiguates protocol vs server version - OPERATIONS: intro reframed from 'Phase 13 (Hosted SaaS)' to self-hosted production operations; missing TOC entries for §6/§7; /v1 note - QUICK_REFERENCE: /v1 note + footer date All numbers measured (494 unit + 255 integration Node at time of writing; 301 Python collected); scopes table left unchanged — there is no 'admin' API-key scope (admin uses ADMIN_TOKEN), verified against src/server.js. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
surpradhan
left a comment
There was a problem hiding this comment.
Independent validation report (pr-validator)
Verdict: Approve. Docs-only PR; every load-bearing claim was probed empirically against the branch worktree (09b28fa). Zero actionable items; one optional nit.
Scope
gh pr diff 162 --name-only = exactly README.md, SETUP.md, OPERATIONS.md, docs/QUICK_REFERENCE.md — matches the stated docs-only scope. Worktree clean and identical to the PR head.
Live server probes (booted from the branch, temp DB, ephemeral port)
GET /health→{"ok":true,"service":"aep-ingest","version":"1.0.0","checks":{"db":"ok"}}— byte-identical to SETUP's updated example.SERVER_VERSIONtraced topackage.json1.0.0(src/server.js:6), confirming the 0.2.0→1.0.0 fix.GET /v1/sessionsno auth (withDASHBOARD_TOKENset) → 401 +API-Version: 1header.GET /sessions→ same 401 + header (backward-compat path live).- Dev-mode nuance verified too: with
DASHBOARD_TOKENunset, unauth reads return 200 (persrc/auth.jsfail-open-in-dev) — consistent with existing docs, not contradicted by this PR. GET /v1/health→ 404 andPOST /v1/eventsno auth → 401 — matches the note that infra probes are unversioned while consumer endpoints are.- Source cross-check:
API-Versionheader set globally (src/server.js:197); v1 router mounted at/v1and/(:1904-1908);/health,/ready,/metrics/prometheus,/dashboard,/docs,/openapi.json,/admin/*all app-level (unversioned) — every endpoint the notes classify is classified correctly, incl. the subtle/metrics(versioned,v1.get) vs/metrics/prometheus(unversioned) split.
Test-count claims (measured on this branch)
npm run test:unit→ 494 pass / 0 fail;npm run test:integration→ 264 pass / 0 fail → 758 total. README's badge700+and "700+ (490+ unit + 250+ integration)" hold, and the resilient phrasing already absorbed drift since authoring (255→264 integration).- Python SDK: pytest collect → 281 unit + 20 integration = 301 — "300+ (280+ unit + 20 integration)" holds exactly.
npm run lint→ clean.
Staleness sweep of the four files
- No leftover
82 tests,200+,<PORT>; all remaining0.2.0mentions are protocol-spec (specversion), which is correct — SETUP/QUICK_REFERENCE headers now cleanly separate protocol vs server version. validScopes = ["read","write"](src/server.js:1674) confirms the deliberate non-change to the scopes table (noadminAPI-key scope exists).
Markdown sanity
Programmatic anchor check over OPERATIONS.md: all internal TOC links resolve, including the new #6-webhooks--alerts-phase-16 / #7-export--cold-storage-phase-17 (consistent with the existing #3-projects-tiers--quotas style).
CI
gh pr checks 162 → 14/14 pass (incl. Required checks in sync).
Findings
- Nit (optional, non-blocking) —
OPERATIONS.md:486: one residual "Phase 13 (Hosted SaaS groundwork) is complete" in §5's body. The intro reframe (the PR's stated scope) is done; this line is historical context inside a section still titled "Phase 13 production checklist", so it reads as intentional history rather than framing. Fine to leave or sweep in a follow-up.
Validation environment cleaned up (node_modules symlink removed, temp DB/logs deleted, worktree clean).
surpradhan
left a comment
There was a problem hiding this comment.
Independent code review — PR #162 (docs: propagate /v1 API versioning + fix stale counts and framing)
Reviewed against source on the rebased branch (09b28fa, parent = current main cdc478f), with a live-server probe and full local gate run.
Verification performed
- /v1 claims vs
src/server.js: verified thev1router is mounted at both/v1and/(lines 1905/1908), theAPI-Version: 1header middleware is global (line 197), and infra/UI/admin routes stay onapp(lines 203–204, 310, 335, 1343, 1667+). Empirically probed a live server (scratch DB, port 8899, cleaned up):/v1/sessions→200,/sessions→200,/v1/health→404,/v1/admin/keys→404,/v1/metrics/prometheus→404, andAPI-Version: 1present on every response including/health,/admin/*, and 404s. All four docs' versioning notes match observed behavior. - openapi.json as spec: confirmed the
serversblock lists/v1as recommended + root as legacy, with per-path root-only overrides for infra probes (e.g./health) — consistent with the doc notes. - Consistency with in-flight PR #161 (VERSIONING.md): README's unversioned list (
/health,/ready,/metrics/prometheus,/admin/*,/dashboard,/docs,/openapi.json) matches #161's list exactly. No contradiction between the two in-flight docs. - SETUP /health example: byte-accurate vs the live handler:
{"ok":true,"service":"aep-ingest","version":"1.0.0","checks":{"db":"ok"}}(SERVER_VERSION= package.json1.0.0). The protocol-vs-build disambiguation in the SETUP header is correct. - Test counts on THIS branch post-rebase: Node
node --test: 494 unit + 264 integration = 758, all pass → badge700+,490+ unit,250+ integrationall hold (integration grew 255→264 after the rebase over #157's globbed runner; the resilient phrasing absorbs it). Pythonpytest --collect-only: 281 unit + 20 integration = 301 →300+ (280+ unit + 20 integration)is exactly right. Integration tests do cover /v1 (server.test.js:4439"API versioning — /v1 prefix and API-Version header"), so the README parenthetical is accurate. - OPERATIONS reframing: §5 "Phase 13 production checklist" heading + body ("Phase 13 … is complete") read as historical provenance, same as §6 "(Phase 16)" / §7 "(Phase 17)" — no contradiction with the neutral intro or the README "built but parked" note. New TOC anchors
#6-webhooks--alerts-phase-16/#7-export--cold-storage-phase-17match GitHub's anchor generation for the existing headings. - Rebase artifacts: none — no conflict markers, each versioning note appears exactly once per file, no duplication against #156/#159's README changes.
- PR-body "deliberately unchanged" claim: correct —
src/server.js:1674validScopes = ["read", "write"]; noadminAPI-key scope exists. - Gates:
npm run lintclean; 494u + 264i pass locally; all 14 CI checks green.
Findings
Nit 1 — README.md ~332–337 (API Response Formats): mixed path styles within one list. The 202 and first 200 example were updated to POST /v1/events / GET /v1/sessions, but the next entries in the same list stay unversioned (GET /sessions/{sessionId}/events, GET /sessions/{sessionId}/audit-bundle, GET /workflows/{traceId}/audit-bundle). Both work (note explains equivalence), but pick one style — suggest converting the remaining three to /v1 to match the "recommended" framing.
Nit 2 — SETUP.md ~536 (versioning note): the unversioned carve-out omits /openapi.json, which README's note and #161's VERSIONING.md both list. Suggest: "…the dashboard/docs UI (/dashboard, /docs, /openapi.json), and /admin/* are not versioned." Keeps the two in-flight docs and README saying the same thing.
Nit 3 — OPERATIONS.md ~22–25 (versioning note): "…is also served under the /v1 prefix with an API-Version: 1 response header, while /admin/* and the infra probes … are unversioned" reads as if the header is specific to /v1-served endpoints. Verified live: the header is set by global middleware on every response, including /admin/* and the infra probes. The other three docs say "every response includes an API-Version: 1 header" — align OPERATIONS with that, e.g. "…is also served under the /v1 prefix (all responses carry an API-Version: 1 header), while /admin/* and the infra probes … are unversioned."
Non-blocking observation (PR body only, not the diff): the body says "measured: 494 unit + 255 integration at time of writing" — post-rebase it's 264 integration. The doc text's 250+ phrasing is unaffected; optionally refresh the body number on the next push.
What looks good
- Every factual claim in the diff checks out against source or live behavior — including the /health shape, the /v1 routing matrix, the header semantics, and every count (Node 758, Python 301, Go 87 vs
80+). - The resilient "N+" phrasing is the right call for counts that CI keeps growing.
- The scopes-table "false alarm" judgment (leaving QUICK_REFERENCE unchanged) is correct and well-reasoned.
- Unversioned-endpoint lists are consistent with the in-flight VERSIONING.md (#161), so the two PRs can land in either order without contradicting each other.
Verdict: Approve (3 nits above; no Critical or Suggestion-severity findings)
Independent review session — reviewer is not the author and will not merge.
…lobal header wording, Phase-13 history phrasing Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All four round-1 nits applied in 8735401: README's remaining three response-format examples converted to /v1 (consistent style); SETUP's carve-out now names /dashboard, /docs, /openapi.json; OPERATIONS reworded so the API-Version header reads as global, not /v1-specific; §5's residual 'Phase 13 (Hosted SaaS groundwork) is complete' recast as infrastructure history with the checklist framed as current for any self-hosted deployment. PR body count refreshed (264 integration post-rebase). |
surpradhan
left a comment
There was a problem hiding this comment.
Validator round 2 — delta 09b28fa..8735401 (final validation)
Verdict: Approve. All four round-1 findings are fixed; no new issues introduced.
Delta scope
git diff 09b28fa..8735401 touches only README.md (4 lines), SETUP.md (2 lines), OPERATIONS.md (11 lines) — exactly the four requested changes, nothing else:
- README §API Response Formats — the two remaining unversioned example headers (
GET /sessions/{sessionId}/events,GET /sessions/{sessionId}/audit-bundle+GET /workflows/{traceId}/audit-bundle) now use/v1. All five endpoint examples in the section are now consistently/v1; the 400/401/403 examples name no endpoints. ✅ - SETUP.md carve-out — dashboard/docs UI exclusion now enumerates
/dashboard,/docs,/openapi.json. ✅ - OPERATIONS.md header wording — decouples the
API-Version: 1header from the/v1prefix ('all responses carry'). ✅ and more accurate than before: the header is set by a globalapp.usein src/server.js:197 registered ahead of all routes. - OPERATIONS.md §5 Phase-13 line (my round-1 nit) — reframed as shipped infrastructure + still-current self-hosted checklist. ✅
Empirical probes (live server boot from the PR worktree)
POST /v1/events→ 202{"accepted":true,"duplicate":false,"id":"evt_..."}(matches README example byte-for-shape)GET /v1/sessions→ 200,GET /v1/sessions/{id}/events→ 200,GET /v1/sessions/{id}/audit-bundle→ 200,GET /v1/workflows/{traceId}/audit-bundle→ 200 (withAUDIT_SIGNING_SECRETset)API-Version: 1header confirmed on/health,/dashboard, and/v1/sessions— the strengthened 'all responses' wording holds, including on unversioned infra/UI routes.- Note: src/openapi.json intentionally documents only the unversioned canonical paths (0
/v1entries, per PR #141 design); route existence verified against src/server.js v1 router (mounted at both/v1and/) and live HTTP.
CI
All 14/14 checks green on head 8735401 (Operator unit tests, Postgres parity, Node 20/22 SDK, Python 3.10–3.13, Go SDK, otelbridge, Required checks in sync, build, docker, test 20.x).
Zero actionable items remaining. Docs-only change, head matches validated worktree, working tree clean.
surpradhan
left a comment
There was a problem hiding this comment.
Round-2 review (delta 09b28fa..8735401) — code-reviewer
Verified each of the four round-1/validator findings against the delta commit 8735401 and the server source:
- README mixed /v1 example styles — FIXED. The response-structure examples block (README.md lines 321–336) is now uniformly
/v1:POST /v1/events,GET /v1/sessions,GET /v1/sessions/{sessionId}/events, and both audit-bundle examples. Remaining unversioned root paths (quickstartPOST /admin/keys, the architecture diagram'sPOST /events) are correct as-is — admin is genuinely unversioned and the versioning note explicitly documents root-path support. - SETUP carve-out missing /openapi.json — FIXED. The versioning note now lists
/dashboard,/docs,/openapi.jsonexplicitly. Verified againstsrc/server.js: all three are mounted directly onapp(lines 359/369/399), not on thev1router, so the carve-out is code-accurate. - OPERATIONS header wording implying API-Version is /v1-specific — FIXED. Now reads "all responses carry an
API-Version: 1header." Verified: the header middleware (src/server.js:197) is a bareapp.useregistered before every route registration (first route is/healthat line 310), so admin, infra probes, dashboard, and both versioned/unversioned consumer routes all get the header. The claim is accurate, not an overclaim. - Residual "Phase 13 (Hosted SaaS groundwork) is complete" in §5 body — FIXED. Reworded to "The Phase 13 infrastructure shipped in full — … — and this checklist remains current for any self-hosted production deployment." This does not overclaim: it drops the SaaS framing, the PR-B–E enumeration is factual (all merged), and the checklist items themselves are untouched and remain true (verified spot-checks: v2-only signature enforcement,
QUOTA_ENFORCEMENT,npm run pruneall match source).
No new inaccuracies introduced. Delta touches only OPERATIONS.md/README.md/SETUP.md (9+/8−); files are NUL-clean UTF-8; no trailing-whitespace or formatting regressions.
Gates: npm run lint clean; npm test 264/264 pass (unit + integration).
Zero actionable items. Verdict: Approve.
(Posted as comment — reviewer shares the author's gh identity, so a formal Approve review is not possible; treat this as the round-2 APPROVE.)
What does this PR do?
Brings four docs in line with shipped reality:
/v1prefix +API-Version: 1header, backward-compatible unversioned paths, and unversioned infra endpoints.Tests: 200+→700+;82 tests (55+27)→ resilient 700+ phrasing (measured: 494 unit + 264 integration at time of writing); Python block118→ 300+."version": "0.2.0"— the server reports the package build1.0.0(verified against src/server.js); header now separates protocol vs server version.Deliberately unchanged: QUICK_REFERENCE's scopes table (reported as missing an
adminscope — false alarm: no such API-key scope exists; admin auth is the separateADMIN_TOKENbearer, per src/server.js validScopes).Why?
CTO review Wave 1, item 7 — the API's versioning story existed only in code and openapi.json, and multiple stale claims (counts, versions, phase framing) contradicted the repo.
How to test?
Every number and path is verifiable in-repo; test counts reproducible via
node --test tests/unit/*.test.js/tests/integration/*.test.js.Checklist
🤖 Generated with Claude Code