chore(api): sebuf migration follow-ups (post-#3242)#3287
chore(api): sebuf migration follow-ups (post-#3242)#3287SebastienMelki merged 12 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@koala73 — checklist item 1 (#3279) landed in What
Per-call-site analysis (not class-level) keeps the trade/index.ts pattern clean — Wired into:
Three latent HIGH(new) #1-class bugs the lint surfaced — all fixed
All three swap to Verification
Caveat (same as before)I haven't manually e2e-tested the three fixed panels in a signed-in browser session — that needs a Clerk pro session I can't drive from terminal. Adding the same checkbox to your pre-merge test plan as last round. Next up#3277 — |
|
@koala73 — checklist item 2 (#3277) landed in Module-scoped Test seam: New test in
Next up: #3278 — |
|
@koala73 — checklist item 3 (#3278) landed in
Runs via Verified:
Next up: checklist items 4+5 — |
|
@koala73 — checklist items 4+5 landed in Proto change
Handler changeCoercion Wire contract diff (partner-facing)
CodegenScoped Tests
Next upChecklist item 6 — SSRF DNS-rebinding sanity-check on the delivery worker. Then 7 ( |
|
@koala73 — checklist item 6 (SSRF DNS-rebinding sanity-check) — FindingNo webhook delivery worker for shipping/v2 exists in this repo. I grepped the entire tree (excluding generated/dist/public/node_modules):
The What I did
CaveatI deliberately did NOT build the delivery worker /
When the delivery worker lands (or already exists in Railway), it MUST import from Status of the full checklist
Plus the optional post-merge prod smoke on the 5 alias URLs that you mentioned — I'll do that after this lands. Promoting from draft → ready when you get a chance to look. Three real bugs found during the work (PREMIUM_RPC_PATHS lint surfaced two, plus the alertThreshold: 0 silent intent-drop), one tracking issue filed (#3288), one comment-refresh that documents the delivery contract. |
|
@koala73 — bonus: prod smoke on the 5 #3242 alias URLs done. ✅ Acceptance from your merge note was "old → 200 via rewrite, not 404." All five alias URLs route in prod — none 404.
All 401 with body Caveat on the smoke being degenerate: For full PRO end-to-end I'd need a signed-in browser session or a real WORLDMONITOR_API_KEY — neither is available from this terminal. Leaving that to your manual pre-merge plan if needed. PR is now ready for review. |
Greptile SummaryThis PR works through the post-#3242 checklist: it adds the
Confidence Score: 4/5Safe to merge after verifying the NEG_TTL test call site — the rest of the checklist items are solid. Prior P1/P2 findings from the #3242 review are fully addressed. One new P1 concern: the NEG_TTL test passes the outer-scope
|
| Filename | Overview |
|---|---|
| scripts/enforce-premium-fetch.mjs | New lint script using TypeScript AST + dynamic import to enforce { fetch: premiumFetch } on all ServiceClient instantiations that call premium-gated paths; correctness depends on exact source-text match for premiumFetch identifier which won't recognise aliased imports. |
| scripts/enforce-rate-limit-policies.mjs | Migrated from regex-parse to dynamic import() + proper YAML parsing; fixes the two silent-drift classes called out in the previous review and now runs under tsx for TypeScript transparency. |
| server/worldmonitor/military/v1/list-military-flights.ts | Adds 30 s module-level negative cache (staleNegUntil) for the stale Redis key after empty/failed reads, with a test seam (_resetStaleNegativeCacheForTests); logic is sound, though the new test may not be passing the right request object. |
| server/worldmonitor/shipping/v2/register-webhook.ts | Correct fix for alertThreshold 0/undefined ambiguity — switches from > 0 ? : 50 to ?? 50 after making the proto field optional int32; range guard kept for belt-and-suspenders. |
| server/worldmonitor/shipping/v2/webhook-shared.ts | DNS-rebinding SSRF guard upgraded from a brief note to a four-step procedure, but no delivery worker exists yet — the guard is purely documentation until #3288 lands. |
| tests/redis-caching.test.mjs | New NEG_TTL test verifies stale-key suppression correctly but passes the outer-scope request fixture rather than the locally constructed ctx URL, so the call site may not test the intended URL path. |
| tests/shipping-v2-handler.test.mjs | Added tests for explicit alertThreshold 0 preservation, negative alertThreshold rejection, and omitted alertThreshold defaulting to 50 — all exercising the ?? 50 fix correctly. |
| .github/workflows/lint-code.yml | Added lint:rate-limit-policies and lint:premium-fetch to CI, resolving the prior review concern about pre-push-only coverage. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[enforce-premium-fetch.mjs] -->|dynamic import| B[src/shared/premium-paths.ts\nPREMIUM_RPC_PATHS Set]
A -->|TypeScript AST walk| C[src/generated/client/\nServiceClient classes]
A -->|AST scan| D[src/ files\nexcl. generated]
D -->|new XServiceClient call| E{fetch option\n=== 'premiumFetch'?}
E -->|Yes| F[✓ Pass]
E -->|No + premium method called| G[✗ Fail lint]
H[enforce-rate-limit-policies.mjs] -->|tsx dynamic import| I[server/_shared/rate-limit.ts\nENDPOINT_RATE_POLICIES]
H -->|yaml.parse| J[docs/api/*.openapi.yaml\npath keys]
I --> K{Policy key\nin OpenAPI paths?}
J --> K
K -->|Yes| L[✓ Pass]
K -->|No| M[✗ Fail lint]
N[RegisterWebhook handler] -->|req.alertThreshold ?? 50| O{undefined?}
O -->|Yes - field omitted| P[default = 50]
O -->|No - explicit value| Q{0..100 range check}
Q -->|Valid| R[Store alertThreshold]
Q -->|Invalid| S[ValidationError]
Reviews (3): Last reviewed commit: "chore(codegen): regenerate unified OpenA..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function loadPremiumPaths() { | ||
| const src = readFileSync(PREMIUM_PATHS_SRC, 'utf8'); | ||
| const re = /'(\/api\/[^']+)'/g; | ||
| const paths = new Set(); | ||
| let m; | ||
| while ((m = re.exec(src)) !== null) paths.add(m[1]); | ||
| if (paths.size === 0) { | ||
| throw new Error(`No premium paths parsed from ${PREMIUM_PATHS_SRC}`); | ||
| } | ||
| return paths; | ||
| } |
There was a problem hiding this comment.
premium-paths.ts vs. live import
loadPremiumPaths() regex-parses src/shared/premium-paths.ts with /'(\/api\/[^']+)'/g — the exact fragility that motivated refactoring enforce-rate-limit-policies.mjs from regex to import(). If premium-paths.ts is reformatted to use double-quoted strings, array spread, or a const assigned from a helper, paths will be silently dropped from the set and the lint will stop catching violations for those endpoints. The two lint scripts are now inconsistent in their resilience to source formatting changes.
| function loadClientClassMap() { | ||
| const map = new Map(); | ||
| walk(GEN_CLIENT_DIR, (file) => { | ||
| if (basename(file) !== 'service_client.ts') return; | ||
| const src = readFileSync(file, 'utf8'); | ||
| const classRe = /export class (\w+ServiceClient)\s*\{/g; | ||
| const classMatch = classRe.exec(src); | ||
| if (!classMatch) return; | ||
| const className = classMatch[1]; | ||
| const methods = new Map(); | ||
| const methodRe = /async (\w+)\s*\([^)]*\)\s*:\s*Promise<[^>]+>\s*\{\s*let path = "([^"]+)"/g; | ||
| let mm; | ||
| while ((mm = methodRe.exec(src)) !== null) { | ||
| methods.set(mm[1], mm[2]); | ||
| } | ||
| map.set(className, methods); | ||
| }); | ||
| if (map.size === 0) { | ||
| throw new Error(`No ServiceClient classes parsed from ${GEN_CLIENT_DIR}`); | ||
| } | ||
| return map; | ||
| } |
There was a problem hiding this comment.
loadClientClassMap stops at first class per file
classRe.exec(src) is called once and not looped — so only the first export class ...ServiceClient per file is registered. Any file that defines a second client class will silently miss it. The generated files currently each contain exactly one class, so this doesn't cause a failure today, but it's a latent gap if the codegen template changes.
Pre-push hook ran lint:rate-limit-policies but the CI workflow did not, so fork PRs and --no-verify pushes bypassed the exact drift check the lint was added to enforce (closes #3278). Adding it right after lint:api-contract so it runs in the same context the lint was designed for.
…le P2 #3287) Two fragilities greptile flagged on enforce-premium-fetch.mjs: 1. loadPremiumPaths regex-parsed src/shared/premium-paths.ts with /'(\/api\/[^']+)'/g — same class of silent drift we just removed from enforce-rate-limit-policies in #3278. Reformatting the source Set (double quotes, spread, helper-computed entries) would drop paths from the lint while leaving the runtime untouched. Fix: flip the shebang to `#!/usr/bin/env -S npx tsx` and dynamic-import PREMIUM_RPC_PATHS directly, mirroring the rate-limit pattern. package.json lint:premium-fetch now invokes via tsx too so the npm-script path matches direct execution. 2. loadClientClassMap ran classRe.exec once, silently dropping every ServiceClient after the first if a file ever contained more than one. Current codegen emits one class per file so this was latent, but a template change would ship un-linted classes. Fix: collect every class-open match with matchAll, slice each class body with the next class's start as the boundary, and scan methods per-body so method-to-class binding stays correct even with multiple classes per file. Verification: - lint:premium-fetch clean (34 classes / 28 premium paths / 466 files — identical counts to pre-refactor, so no coverage regression). - Negative test: revert src/services/economic/index.ts to globalThis.fetch → exit 1 with file:line, bound var name, and premium method list (getNationalDebt). Restore → clean. - lint:rate-limit-policies still clean.
|
@koala73 — greptile P1 addressed in
|
|
@koala73 — greptile P2s on Two fragilities collapsed(a) Regex → (b) Verification
All three greptile findings now closed; the two lint scripts are now symmetric in their resilience to source reformatting. Ready for another look. |
Review — no blockersApprove once you've decided on the nits. The three ServiceClient fixes (correlation-engine / economic / sanctions-pressure) alone justify the merge — all three were silently 401-ing for signed-in browser pros on premium methods. Proposed improvements (all optional, none blocking)1. Range enforcement now lives entirely at the wire layer via const alertThreshold = req.alertThreshold ?? 50;
if (alertThreshold < 0 || alertThreshold > 100) {
throw new ValidationError([{ field: 'alertThreshold', description: 'must be 0..100' }]);
}2.
3. OpenAPI parser in
4.
The MDX isn't in the diff. The commit message cites #3288 — put the issue number in the comment directly (or update the MDX in this PR) so a future reader can follow it. 5. The walker scans the full AST for 6. Shebang
Pre-merge checks I'd want
|
SebastienMelki
force-pushed
the
feat/sebuf-followups-3242
branch
from
857e170 to
1926fc5
Compare
Pre-push hook ran lint:rate-limit-policies but the CI workflow did not, so fork PRs and --no-verify pushes bypassed the exact drift check the lint was added to enforce (closes #3278). Adding it right after lint:api-contract so it runs in the same context the lint was designed for.
…le P2 #3287) Two fragilities greptile flagged on enforce-premium-fetch.mjs: 1. loadPremiumPaths regex-parsed src/shared/premium-paths.ts with /'(\/api\/[^']+)'/g — same class of silent drift we just removed from enforce-rate-limit-policies in #3278. Reformatting the source Set (double quotes, spread, helper-computed entries) would drop paths from the lint while leaving the runtime untouched. Fix: flip the shebang to `#!/usr/bin/env -S npx tsx` and dynamic-import PREMIUM_RPC_PATHS directly, mirroring the rate-limit pattern. package.json lint:premium-fetch now invokes via tsx too so the npm-script path matches direct execution. 2. loadClientClassMap ran classRe.exec once, silently dropping every ServiceClient after the first if a file ever contained more than one. Current codegen emits one class per file so this was latent, but a template change would ship un-linted classes. Fix: collect every class-open match with matchAll, slice each class body with the next class's start as the boundary, and scan methods per-body so method-to-class binding stays correct even with multiple classes per file. Verification: - lint:premium-fetch clean (34 classes / 28 premium paths / 466 files — identical counts to pre-refactor, so no coverage regression). - Negative test: revert src/services/economic/index.ts to globalThis.fetch → exit 1 with file:line, bound var name, and premium method list (getNationalDebt). Restore → clean. - lint:rate-limit-policies still clean.
… nit 1 #3287) Wire-layer buf.validate enforces 0..100, but direct handler invocation (internal jobs, test harnesses, future transports) bypasses it. Cheap invariant-at-the-boundary — rejects < 0 or > 100 with ValidationError before the record is stored. Tests: restored the rejects-out-of-range cases that were dropped when the branch was (correctly) deleted as dead code on the previous commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…#3287) loadClientClassMap: The method regex `async (\w+)\s*\([^)]*\)\s*:\s*Promise<[^>]+>\s*\{\s*let path = "..."` assumed (a) no nested `)` in arg types, (b) no nested `>` in the return type, (c) `let path = "..."` as the literal first statement. Any codegen template shift would silently drop methods with the lint still passing clean — the same silent-drift class #3287 just closed on the premium-paths side. Now walks the service_client.ts AST, matches `export class *ServiceClient`, iterates `MethodDeclaration` members, and reads the first `let path: string = '...'` variable statement as a StringLiteral. Tolerant to any reformatting of arg/return types or method shape. findCalls scope-blindness: Added limitation comment — the walker matches `<varName>.<method>()` anywhere in the file without respecting scope. Two constructions in different function scopes sharing a var name merge their called-method sets. No current src/ file hits this; the lint errs cautiously (flags both instances). Keeping the walker simple until scope-aware binding is needed. webhook-shared.ts: Inlined issue reference (#3288) so the breadcrumb resolves without bouncing through an MDX that isn't in the diff. Verification: - lint:premium-fetch clean — 34 classes / 28 premium paths / 489 files. Pre-refactor: 34 / 28 / 466. Class + path counts identical; file bump is from the main-branch rebase, not the refactor. - Negative test: revert src/services/economic/index.ts premiumFetch → globalThis.fetch. Lint exits 1 at `src/services/economic/index.ts:64:7` with `premium method(s) called: getNationalDebt`. Restore → clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…#3287) Input side (ENDPOINT_RATE_POLICIES) was flipped to live `import()` in 4e79d02. Output side (OpenAPI routes) still regex-scraped top-level `paths:` keys with `/^\s{4}(\/api\/[^\s:]+):/gm` — hard-coded 4-space indent. Any YAML formatter change (2-space indent, flow style, line folding) would silently drop routes and let policy-drift slip through — same silent-drift class the input-side fix closed. Now uses the `yaml` package (already a dep) to parse each .openapi.yaml and reads `doc.paths` directly. Verification: - Clean: 6 policies / 189 routes (was 182 — yaml parser picks up a handful the regex missed, closing a silent coverage gap). - Negative test: rename policy key back to /api/sanctions/v1/lookup-entity → exits 1 with the same incident-attributed remedy. Restore → clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pre-push on main now runs `make generate` and fails the push if there's drift. This branch rebased on main and the hook surfaced legit codegen improvements main hasn't regenerated: enum query params default to `*_UNSPECIFIED` sentinel values (not `""`) and `repeated` query fields iterate properly with `params.append(...)` per value. No proto definition changes — just regenerated output from the current toolchain. No @ts-nocheck loss (verified). Scope covers every service that has a RPC using an enum query param or a repeated primitive query field, hence the breadth: aviation, climate, conflict, cyber, economic, health, infrastructure, intelligence, market, military, positive_events, resilience, scenario, shipping, supply_chain, trade, unrest. Isolated from the #3287 nit fixes so the review diff for those stays narrow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@koala73 — all 5 addressable nits from your review landed. ✅
Highlights per nit1. Handler-side guard restored. 2. Method regex → 3. OpenAPI regex → 4. Inlined #3288 — no more MDX bounce. 5. Scope-blindness comment — documents that the walker errs cautiously (merges called-method sets across functions sharing a var name) and keeps the walker simple until scope-aware binding is actually needed. Codegen sync (separate commit —
|
…nal-helper justification Carried in from #3248 merge as a band-aid (called out in #3242 review followup checklist item 7). The endpoint genuinely belongs in internal-helper — RELAY_SHARED_SECRET-bearer auth, cron-only caller, never reached by dashboards or partners. Same shape constraint as api/notify.ts. Replaces the apologetic "filed here to keep the lint green" framing with a proper structural justification: modeling it as a generated service would publish internal cron plumbing as user-facing API surface. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds scripts/enforce-premium-fetch.mjs — AST-walks src/, finds every
`new <ServiceClient>(...)` (variable decl OR `this.foo =` assignment),
tracks which methods each instance actually calls, and fails if any
called method targets a path in src/shared/premium-paths.ts
PREMIUM_RPC_PATHS without `{ fetch: premiumFetch }` on the constructor.
Per-call-site analysis (not class-level) keeps the trade/index.ts pattern
clean — publicClient with globalThis.fetch + premiumClient with
premiumFetch on the same TradeServiceClient class — since publicClient
never calls a premium method.
Wired into:
- npm run lint:premium-fetch
- .husky/pre-push (right after lint:rate-limit-policies)
- .github/workflows/lint-code.yml (right after lint:api-contract)
Found and fixed three latent instances of the HIGH(new) #1 class from
#3242 review (silent 401 → empty fallback for signed-in browser pros):
- src/services/correlation-engine/engine.ts — IntelligenceServiceClient
built with no fetch option called deductSituation. LLM-assessment overlay
on convergence cards never landed for browser pros without a WM key.
- src/services/economic/index.ts — EconomicServiceClient with
globalThis.fetch called getNationalDebt. National-debt panel rendered
empty for browser pros.
- src/services/sanctions-pressure.ts — SanctionsServiceClient with
globalThis.fetch called listSanctionsPressure. Sanctions-pressure panel
rendered empty for browser pros.
All three swap to premiumFetch (single shared client, mirrors the
supply-chain/index.ts justification — premiumFetch no-ops safely on
public methods, so the public methods on those clients keep working).
Verification:
- lint:premium-fetch clean (34 ServiceClient classes, 28 premium paths,
466 src/ files analyzed)
- Negative test: revert any of the three to globalThis.fetch → exit 1
with file:line and called-premium-method names
- typecheck + typecheck:api clean
- lint:api-contract / lint:rate-limit-policies / lint:boundaries clean
- tests/sanctions-pressure.test.mjs + premium-fetch.test.mts: 16/16 pass
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The legacy /api/military-flights handler had NEG_TTL = 30_000ms — a short suppression window after a failed live + stale read so we don't Redis-hammer the stale key during sustained relay+seed outages. Carried into the sebuf list-military-flights handler: - Module-scoped `staleNegUntil` timestamp (per-isolate on Vercel Edge, which is fine — each warm isolate gets its own 30s suppression window). - Set whenever fetchStaleFallback returns null (key missing, parse fail, empty array after staleToProto filter, or thrown error). - Checked at the entry of fetchStaleFallback before doing the Redis read. - Test seam `_resetStaleNegativeCacheForTests()` exposed for unit tests. Test pinned in tests/redis-caching.test.mjs: drives a stale-empty cycle three times — first read hits Redis, second within window doesn't, after test-only reset it does again. Verified: 18/18 redis-caching tests pass, typecheck:api clean, lint:premium-fetch clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous lint regex-parsed ENDPOINT_RATE_POLICIES from the source file. That worked because the literal happens to fit a single line per key today, but a future reformat (multi-line key wrap, formatter swap, etc.) would silently break the lint without breaking the build — exactly the failure mode that's worse than no lint at all. Fix: - Export ENDPOINT_RATE_POLICIES from server/_shared/rate-limit.ts. - Convert scripts/enforce-rate-limit-policies.mjs to async + dynamic import() of the policy object directly. Same TS module that the gateway uses at runtime → no source-of-truth drift possible. - Run via tsx (already a dev dep, used by test:data) so the .mjs shebang can resolve a .ts import. - npm script swapped to `tsx scripts/...`. .husky/pre-push uses `npm run lint:rate-limit-policies` so no hook change needed. Verified: - Clean: 6 policies / 182 gateway routes. - Negative test (rename a key to the original sanctions typo /api/sanctions/v1/lookup-entity): exit 1 with the same incident- attributed remedy message as before. - Reformat test (split a single-line entry across multiple lines): still passes — the property is what's read, not the source layout. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ranch (#3242 followup) Before: alert_threshold was a plain int32. proto3 scalar default is 0, so the handler couldn't distinguish "partner explicitly sent 0 (deliver every disruption)" from "partner omitted the field (apply legacy default 50)" — both arrived as 0 and got coerced to 50 by `> 0 ? : 50`. Silent intent-drop for any partner who wanted every alert. The subsequent `alertThreshold < 0` branch was also unreachable after that coercion. After: - Proto field is `optional int32 alert_threshold` — TS type becomes `alertThreshold?: number`, so omitted = undefined and explicit 0 stays 0. - Handler uses `req.alertThreshold ?? 50` — undefined → 50, any number passes through unchanged. - Dead `< 0 || > 100` runtime check removed; buf.validate `int32.gte = 0, int32.lte = 100` already enforces the range at the wire layer. Partner wire contract: identical for the omit-field and 1..100 cases. Only behavioural change is explicit 0 — previously impossible to request, now honored per proto3 optional semantics. Scoped `buf generate --path worldmonitor/shipping/v2` to avoid the full- regen `@ts-nocheck` drift Seb documented in the #3242 PR comments. Re-applied `@ts-nocheck` on the two regenerated files manually. Tests: - `alertThreshold 0 coerces to 50` flipped to `alertThreshold 0 preserved`. - New test: `alertThreshold omitted (undefined) applies legacy default 50`. - `rejects > 100` test removed — proto/wire validation handles it; direct handler calls intentionally bypass wire and the handler no longer carries a redundant runtime range check. Verified: 18/18 shipping-v2-handler tests pass, typecheck + typecheck:api clean, all 4 custom lints clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…inding contract (#3242 followup) #3242 followup checklist item 6 from @koala73 — sanity-check that the delivery worker honors the re-resolve-and-re-check contract that isBlockedCallbackUrl explicitly delegates to it. Audit finding: no delivery worker for shipping/v2 webhooks exists in this repo. Grep across the entire tree (excluding generated/dist) shows the only readers of webhook:sub:* records are the registration / inspection / rotate-secret handlers themselves. No code reads them and POSTs to the stored callbackUrl. The delivery worker is presumed to live in Railway (separate repo) or hasn't been built yet — neither is auditable from this repo. Refreshes the comment block at the top of webhook-shared.ts to: - explicitly state DNS rebinding is NOT mitigated at registration - spell out the four-step contract the delivery worker MUST follow (re-validate URL, dns.lookup, re-check resolved IP against patterns, fetch with resolved IP + Host header preserved) - flag the in-repo gap so anyone landing delivery code can't miss it Tracking the gap as #3288 — acceptance there is "delivery worker imports the patterns + helpers from webhook-shared.ts and applies the four steps before each send." Action moves to wherever the delivery worker actually lives (Railway likely). No code change. Tests + lints unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pre-push hook ran lint:rate-limit-policies but the CI workflow did not, so fork PRs and --no-verify pushes bypassed the exact drift check the lint was added to enforce (closes #3278). Adding it right after lint:api-contract so it runs in the same context the lint was designed for.
…le P2 #3287) Two fragilities greptile flagged on enforce-premium-fetch.mjs: 1. loadPremiumPaths regex-parsed src/shared/premium-paths.ts with /'(\/api\/[^']+)'/g — same class of silent drift we just removed from enforce-rate-limit-policies in #3278. Reformatting the source Set (double quotes, spread, helper-computed entries) would drop paths from the lint while leaving the runtime untouched. Fix: flip the shebang to `#!/usr/bin/env -S npx tsx` and dynamic-import PREMIUM_RPC_PATHS directly, mirroring the rate-limit pattern. package.json lint:premium-fetch now invokes via tsx too so the npm-script path matches direct execution. 2. loadClientClassMap ran classRe.exec once, silently dropping every ServiceClient after the first if a file ever contained more than one. Current codegen emits one class per file so this was latent, but a template change would ship un-linted classes. Fix: collect every class-open match with matchAll, slice each class body with the next class's start as the boundary, and scan methods per-body so method-to-class binding stays correct even with multiple classes per file. Verification: - lint:premium-fetch clean (34 classes / 28 premium paths / 466 files — identical counts to pre-refactor, so no coverage regression). - Negative test: revert src/services/economic/index.ts to globalThis.fetch → exit 1 with file:line, bound var name, and premium method list (getNationalDebt). Restore → clean. - lint:rate-limit-policies still clean.
… nit 1 #3287) Wire-layer buf.validate enforces 0..100, but direct handler invocation (internal jobs, test harnesses, future transports) bypasses it. Cheap invariant-at-the-boundary — rejects < 0 or > 100 with ValidationError before the record is stored. Tests: restored the rejects-out-of-range cases that were dropped when the branch was (correctly) deleted as dead code on the previous commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…#3287) loadClientClassMap: The method regex `async (\w+)\s*\([^)]*\)\s*:\s*Promise<[^>]+>\s*\{\s*let path = "..."` assumed (a) no nested `)` in arg types, (b) no nested `>` in the return type, (c) `let path = "..."` as the literal first statement. Any codegen template shift would silently drop methods with the lint still passing clean — the same silent-drift class #3287 just closed on the premium-paths side. Now walks the service_client.ts AST, matches `export class *ServiceClient`, iterates `MethodDeclaration` members, and reads the first `let path: string = '...'` variable statement as a StringLiteral. Tolerant to any reformatting of arg/return types or method shape. findCalls scope-blindness: Added limitation comment — the walker matches `<varName>.<method>()` anywhere in the file without respecting scope. Two constructions in different function scopes sharing a var name merge their called-method sets. No current src/ file hits this; the lint errs cautiously (flags both instances). Keeping the walker simple until scope-aware binding is needed. webhook-shared.ts: Inlined issue reference (#3288) so the breadcrumb resolves without bouncing through an MDX that isn't in the diff. Verification: - lint:premium-fetch clean — 34 classes / 28 premium paths / 489 files. Pre-refactor: 34 / 28 / 466. Class + path counts identical; file bump is from the main-branch rebase, not the refactor. - Negative test: revert src/services/economic/index.ts premiumFetch → globalThis.fetch. Lint exits 1 at `src/services/economic/index.ts:64:7` with `premium method(s) called: getNationalDebt`. Restore → clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…#3287) Input side (ENDPOINT_RATE_POLICIES) was flipped to live `import()` in 4e79d02. Output side (OpenAPI routes) still regex-scraped top-level `paths:` keys with `/^\s{4}(\/api\/[^\s:]+):/gm` — hard-coded 4-space indent. Any YAML formatter change (2-space indent, flow style, line folding) would silently drop routes and let policy-drift slip through — same silent-drift class the input-side fix closed. Now uses the `yaml` package (already a dep) to parse each .openapi.yaml and reads `doc.paths` directly. Verification: - Clean: 6 policies / 189 routes (was 182 — yaml parser picks up a handful the regex missed, closing a silent coverage gap). - Negative test: rename policy key back to /api/sanctions/v1/lookup-entity → exits 1 with the same incident-attributed remedy. Restore → clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… proto change The shipping/v2 webhook alert_threshold field was flipped from `int32` to `optional int32` with an expanded doc comment in f333946. That comment now surfaces in the unified docs/api/worldmonitor.openapi.yaml bundle (introduced by #3341). Regenerated with sebuf v0.11.1 to pick it up. No behaviour change — bundle-only documentation drift. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SebastienMelki
force-pushed
the
feat/sebuf-followups-3242
branch
from
1926fc5 to
dbe6e53
Compare
|
@koala73 — rebased on main now that #3341 landed. ✅ The earlier One new commit ( Final branch state11 commits — every one scoped to the PR intent, no toolchain noise:
Verification (post-rebase)
Ready. |
|
Hi @greptile-apps please review again |
2 participants
Follow-up to #3242 (sebuf-migration-3207). Tackles the next-PR checklist @koala73 left in the merge approval, in priority order. Opening as draft so each commit lands visibly while the queue works through; will mark ready when the high-leverage items are in.
cc @koala73 — kicking off the next-PR checklist now. Will land each item as its own commit and ping you per-commit per the prior PR rhythm. Pushing back on any item that needs a different shape than the checklist suggests.
Plan (priority-ordered, from your merge note)
ServiceClient↔PREMIUM_RPC_PATHSparity lintfetchStaleFallbacknegative cache (NEG_TTL = 30_000parity)enforce-rate-limit-policies.mjs: regex →import()alertThreshold: 0coercion on commit 8 (optional int32 alert_threshold)alertThreshold < 0branch — removeapi/internal/brief-why-matters.tsmanifest band-aid74c5fe16Post-merge: optional smoke on the 5 v1 alias URLs in prod to confirm Vercel file-based routing picks them up (your suggestion).
Out of scope here (stays deferred to its own PR)
Test plan
npm run lint:api-contractcleannpm run lint:rate-limit-policiescleannpm run lint:boundariescleannpm run lint:premium-fetchclean — to be added in checklist item (1)npm run typecheck+typecheck:apicleanmake generatezero-diff after each proto-touching committests/edge-functions.test.mjs+ relevant handler tests pass🤖 Generated with Claude Code