Repository navigation
Implement the UX and technical review: server-rendered catalog, closed developer funnel, visible pipeline failures - #54
Merged
Merged
Conversation
The gateway charged callers with `void meterInvocation(...)` — a promise
deliberately not awaited — and the DEBIT ledger entry plus the balance
decrement live inside it, in the same transaction as the Invocation row.
Cloud Run allocates CPU per request by default (the `takoapi` service has
no `--no-cpu-throttling` and no min-instances), so once the response is
flushed a still-pending promise is not guaranteed to run. When the instance
is throttled or reclaimed the transaction is cut short and the Invocation
row and the charge disappear together: a 200 was served, the caller paid
nothing, and nothing recorded that it happened. `billing.ts` also swallowed
every failure in an empty `catch {}`, so even the errors that *were* thrown
left no trace.
- split the transaction body out as `writeInvocation`, and expose two
intents on top of it: `debitInvocation` (money — must be awaited) and
`meterInvocation` (telemetry — safe to fire-and-forget). A billed input
handed to `meterInvocation` is delegated to `debitInvocation`, so a
missed call site still charges correctly.
- replace the empty `catch {}` with `console.error`, so a debit that fails
is visible in Cloud Logging instead of vanishing silently. Neither
function throws: a billing failure must not turn a served call into a 500.
- `/v1/agents/[slug]/message` and `/v1/chat/completions` now `await` the
debit before returning when `billedUsd > 0`; free and failed calls stay
fire-and-forget. Cost: one extra Cloud SQL round-trip (unix socket,
single-digit ms) on billed calls only.
BEHAVIOUR CHANGE: billed calls now block on the debit before the response
is returned.
NOTE: this only closes the window it can close from inside the process.
The stream route is handled separately, and both fixes depend on Cloud Run
being given `--no-cpu-throttling` — an operator action, see the PR body.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…der (F2)
The stream route priced a call off `upstream.status` the moment the fetch
promise resolved — before a single SSE byte had been read — and metered it
immediately. Four lines later came:
if (!upstream.body) return 502 "Agent did not return a stream"
so an upstream that answered 200 with no body produced a 502 for the caller
and a paid, status-200 Invocation row on our side. The same root cause hid
every other mid-stream failure: an upstream that broke halfway, a client
that hung up, and the 120s abort all left the early "success" row untouched,
so the stream route's success rate was structurally overstated and could not
be compared with the non-streaming routes, which only price after
`await res.json()`.
Billing now follows the stream's actual outcome:
- no body → `NO_STREAM_BODY`, status 502, billed 0, then the 502.
- otherwise `startInvocation` opens the row unpriced *before* relaying, so a
stream that dies mid-flight still counts in the denominator.
- `settleInvocation` closes that same row when the stream terminates —
updating it rather than inserting a second one, so a stream remains one
Invocation and one callsCount increment. Only a stream that ran to
completion over a <400 response is charged.
- `pull()` is wrapped in try/catch (it had none): a mid-stream break or the
TIMEOUT_MS abort settles as `STREAM_ABORTED`, and `cancel()` settles as
`CLIENT_DISCONNECTED`. Both bill 0, and a `settled` guard keeps the
terminal write exactly-once.
BEHAVIOUR CHANGE, two of them:
1. a partially delivered stream is now free — previously it was charged in
full as soon as the headers looked fine.
2. `Invocation.errorCode` gains three values on this route:
NO_STREAM_BODY / STREAM_ABORTED / CLIENT_DISCONNECTED. No migration —
the column already exists.
NOTE: settlement runs after the response headers are flushed, which makes
this route *more* dependent on Cloud Run keeping CPU allocated than F1 is.
Without `--no-cpu-throttling` this moves the charge to a less reliable
moment rather than a safer one — the two changes must land together with
that operator action. See the PR body.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nt IP
`checkRateLimit` built every bucket as `${config.key}:${extractClientIp(req)}`,
and the three gateway routes pass `key = gw:<apiKeyId>`. The effective bucket
was therefore `gw:<keyId>:<ip>` — one 120/60s allowance *per IP*, not per key.
`extractClientIp` reads the first element of X-Forwarded-For. Cloud Run and the
GCLB append the real client address to any inbound XFF, so that first element is
supplied by the caller; `src/proxy.ts` cannot normalise it because its matcher
excludes `api|v1|mcp` entirely. A caller who varies the header per request gets
a fresh bucket every time, so the per-key limit did not exist.
That limit is load-bearing for money, not just for load: `checkCreditPreflight`
reads the balance and the debit lands later, with no lock in between, and
billing.ts documents the limiter as the thing that bounds that concurrency
("The shared rate limiter bounds that concurrency"). With the limiter bypassable,
concurrent calls can each pass the same pre-flight read and drive a balance
arbitrarily negative.
- `RateLimitConfig` gains `perIp?: boolean` (default true — unchanged behaviour
for the 11 anonymous endpoints, where the IP is the only identifier available).
- the three `/v1` routes pass `perIp: false`, so an authenticated caller gets one
bucket per API key regardless of what they claim their IP is.
- documented on `extractClientIp` that the value is caller-controlled and must
never be used as identity, authorisation input, or evidence.
BEHAVIOUR CHANGE: a single API key is now limited to 120 requests/60s in total
across all source IPs. A legitimate caller spread over many IPs that previously
got 120/min *each* will start seeing 429s. `ApiKey.rateLimit` (wired up in a
following commit) is the per-key escape hatch.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`ApiKey` has carried `scopes`, `rateLimit` and `monthlyQuota` since the gateway shipped, and nothing ever read any of them: all 29 `rateLimit` matches in src/ are imports of the limiter module, none is `keyRecord.rateLimit`. Every key got the literal `max: 120`, so tightening one abusive key meant changing the constant for everybody — and the schema advertised a per-key control that did not exist. - `gatewayRateLimit(key)` in src/lib/apikey.ts returns `ApiKey.rateLimit` when it is a positive number, else `DEFAULT_GATEWAY_RATE_LIMIT` (120, the previous literal — so keys with a null override behave exactly as before). - the three `/v1` routes use it. - `scopes` and `monthlyQuota` are marked `NOT ENFORCED YET` in schema.prisma so the columns stop reading as working guarantees. Wiring them needs product decisions (scope taxonomy, month boundary, what happens at the cap) that do not belong in this PR — flagged for follow-up instead. Schema change is comments only — no migration. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…re failures PAYPAL_ENV=live and TAKO_TOPUP_FEE_PCT=5 on the running revision, so this path handles real money. Three ways it could take a payment and not account for it: 1. The fee could be skipped permanently. `captureAndCredit`'s idempotency probe looks only for `type: "TOPUP"` with the order id, but the gross and the fee were two independent `grantCredit` transactions. Crash between them (instance reclaimed, DB blip) and the gross was credited, the fee was not — then the retry found the TOPUP row, returned "already", and the fee was never charged. Now `applyTopUp` writes both ledger entries and the net balance change in one transaction, so "a TOPUP row exists" honestly means "fully settled". 2. Three `return "failed"`s sat *after* `captureOrder` — i.e. after PayPal may already have taken the buyer's money — for a non-COMPLETED capture (a PENDING eCheck, say), an unattributable payment, and a non-positive amount. None wrote a ledger row, logged, or alerted: money in, no credit, nothing to reconcile against. All of them now log the order id, capture id, user and amount, and the crediting call itself is wrapped so a failed write is reported the same way. 3. The webhook discarded `captureAndCredit`'s return value and answered 200 unconditionally, which told PayPal never to redeliver — throwing away the last chance to recover exactly the cases in (2). BEHAVIOUR CHANGE: the webhook now returns 500 when an approved order could not be credited, so PayPal retries it. `captureAndCredit` is idempotent, so a redelivery that finds the money already credited reports "already" and returns 200. A genuinely unrecoverable order will be retried until PayPal gives up (~3 days) and show up as repeated failures in the webhook dashboard — which is the intent: a visible failure beats a silent one. Successful deliveries now echo the outcome in the response body. NOT fixed here: there is still no `PAYMENT.CAPTURE.COMPLETED` subscriber, so a capture that settles later (case 2's PENDING eCheck) is logged but never credited automatically. That needs a new webhook subscription configured on the PayPal side as well as code — flagged for follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`isAuthorizedCron` also accepted `?key=<secret>`. Cloud Run's request log stores the full request URL, query string included, in the _Default bucket with 30-day retention — so a single `curl ".../api/cron/health?key=$CRON_SECRET"`, the most natural way to hand-test one of these endpoints, would have written the secret into a log readable by anyone holding `logging.viewer`. That is a much wider audience than the Secret Manager binding the value is delivered through, and the secret guards five cron routes, one of which (weekly-digest) sends email. Nothing in production uses the query form: all six Cloud Scheduler jobs send `Authorization`, and no doc or script references `?key=`. So the query path was pure downside — a way to leak the secret, with nothing depending on it. - header-only, and `crypto.timingSafeEqual` in place of `===` (cheap, and the right habit for a secret comparison even where the side channel is not practically exploitable across the public internet). BEHAVIOUR CHANGE: `?key=<secret>` no longer authenticates. Anything hand-rolled against it must move to `-H "Authorization: Bearer <secret>"`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
401 / 402 / 429 are all returned before `meterInvocation` runs, and neither the three `/v1` routes nor `/mcp` go through `withRequestLog` — so a rejected call left no Invocation row and no RequestLog row. Any error or success rate computed from the Invocation table is missing precisely the calls the gateway turned away: the denominator excludes our own refusals. Correcting the premise this was filed under: these responses are *not* invisible today. Cloud Run's platform request log records each one with timestamp, status and URL, retained 30 days in the _Default bucket. The real gap is attribution — that log has no userId, no apiKeyId, no agent, no reason, and 402 and 429 are indistinguishable in it beyond the status code, so none of it joins to Invocation. So this adds attribution rather than existence: `logGatewayRejection` writes one structured line that Cloud Run parses into jsonPayload, carrying `reason` (unauthenticated | rate_limited | insufficient_credit), the route *template* (never an interpolated path — keeps log cardinality bounded), and, where known, apiKeyId / userId / agentSlug. The 402 branch also carries `requiredUsd` and `balanceUsd`, which the credit gate has already computed and which are the only quantification of demand lost to an empty balance. Deliberately stdout, not the DB: wrapping these routes in `withRequestLog` would add a write per rejection to a table that has no retention policy and, until the next commit, stores plaintext IPs — i.e. it would make an unbounded-growth and a privacy problem worse in order to fix an observability one. Logging costs nothing per row and is queryable in Logs Explorer today. No behaviour change: responses, statuses and bodies are identical. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`RequestLog.ip` stored the raw client address, and nothing ever deleted from the
table. The only sweep in the codebase, `maybeClean()` in ratelimit.ts, targets
`RateLimitBucket`; no cron route, script or Scheduler job touched RequestLog. So
plaintext personal data accumulated indefinitely on a db-f1-micro with a 10 GB
disk, with no expiry and no purge path.
The stored value was not even dependable: it is the first element of a
caller-supplied X-Forwarded-For (Cloud Run appends the real address rather than
replacing the header), so it was a privacy liability that could not serve as
evidence either — bad on both counts, which is why hashing alone is not the whole
fix here.
- `hashClientIp` writes a salted SHA-256 truncated to 128 bits, with the UTC date
mixed in, so the same address hashes differently each day. That keeps the one
question the column was good for ("same source?") within a day and drops
cross-day correlation without extra deletion machinery.
- No salt, no value: with TAKO_IP_SALT unset the column is left null and the
process warns once. An unsalted digest over a 32-bit address space is brute-
forceable in seconds, so a reversible hash would be worse than nothing.
- New `/api/cron/retention` route, guarded by `isAuthorizedCron`, deletes rows
older than 90 days in batches of 5k, capped at 20 batches per run so it cannot
overrun `maxDuration`. It reports `exhausted` so a backlog that outlives one run
is visible instead of looking like a clean sweep. PostgreSQL's DELETE takes no
LIMIT, so the batching goes through `ctid IN (SELECT ... LIMIT n)` — the exact
trap F8 flagged for whoever implemented this.
MIGRATION 010 IS DESTRUCTIVE ON PURPOSE: it DROPs "ip" instead of renaming it.
Renaming would keep the plaintext addresses this change exists to remove, under a
name claiming they were hashed. Nothing reads the column (no `.ip` reader in
src/), so it is code-safe — but snapshot the rows first if they are wanted for an
investigation, because after `prisma migrate deploy` they are gone.
Two operator prerequisites, neither doable from a PR: create the TAKO_IP_SALT
secret and bind it to the service, and create the Cloud Scheduler job that calls
this route. Until the salt exists ipHash is null; until the job exists the route
is inert. Both are in the PR body.
NOT fixed here: `userAgent` is still stored in full (truncated to 500 chars).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…est value (F7) `Agent.healthStatus` / `healthCheckedAt` are the only health state there is, and the hourly cron overwrites them in place. There is no history model anywhere in the schema — grep for health in schema.prisma returns exactly those two lines. So "this agent is down right now" was answerable and "since when", "for how long", and "what was its availability last week" were not, for any agent, ever. - new `AgentHealthCheck` (agentId, status, previousStatus, checkedAt) + migration 011. - `runHealthChecks` now selects `healthStatus` alongside the probe targets and appends a row only when the probe disagrees with the stored value. Writing every probe would add (agents x 24) rows a day forever on a 10 GB shared-core instance to answer the same questions; recording flips keeps the table proportional to how often agents actually change, and makes "when did it go down" a single lookup — the `checkedAt` of the newest row with status='down'. - the agent update and the history insert go in one transaction, so the stored status and its history cannot disagree. The existing per-agent error isolation is kept: a failed write increments `writeErrors` (and backs out its `transitions` count) rather than aborting the batch. - `HealthSummary` gains `transitions`, and the cron route logs the summary as one structured line. It was previously returned to Cloud Scheduler, which discards response bodies — so even the per-run totals were being thrown away. No behaviour change to what the endpoint returns, beyond the added field. FOLLOW-UP not done here: this table needs a retention policy of its own. The 90-day sweep added for RequestLog covers only that table. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…all data (F6)
`Skill.downloads` is a number scraped off each skill's clawskills.sh page
(scrape-clawskills.ts takes `texts[4]` by DOM position; daily-sync.ts overwrites
it nightly via the takoapi-daily-sync Scheduler job). Nothing in this repo ever
increments it — install.sh, packages/takoapi-install and takoapi_skill/ report
nothing back — so it carries no information about installs through TakoAPI. It is
also pre-rounded at the source, where "1.2k" becomes 1200.
The trending page nonetheless described it, in all 15 locales, as "the
most-downloaded ... right now — a live leaderboard of what coding agents are
installing". Three claims, none true: it is a T-1 snapshot, not live; it is not
"right now"; and it is not what agents installed, here or anywhere we can see.
- `metaDescription` and `ogDescription` in all 15 locales now attribute the
numbers to clawskills.sh and say "daily" instead of "live". Values only — no key
added, renamed or removed, so `check:i18n` still reports 618 keys across 14
locales matching en.json.
- `Skill.downloads` gets a schema comment stating where the number comes from and
that it must not be summed into a site metric — the admin panel does exactly
that today (`_sum: { downloads: true }` surfaced as `totalDownloads`).
Comment-only schema change; no migration.
DELIBERATELY NOT DONE: renaming the column to `externalDownloads`. It is the
cleaner fix and it is what the finding recommends, but it means a column rename
across ~15 call sites plus a migration, and deploys here are a manual
`gcloud run deploy` with no CI build behind them — during the rollout window the
old revision would be querying a column that no longer exists. Not worth taking
that risk inside a batch of billing changes. Flagged for a PR of its own.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ected This plan is what prompted the verification pass the rest of this PR came out of. It was drafted against a working tree 18 commits behind origin/main, so before committing it its claims were re-checked against origin/main = b868319 and, where they concerned production, against read-only gcloud output. Corrected before landing: - §0.1's "cloudbuild.yaml has drifted from production; running it once would overwrite the production secrets with placeholders" — REFUTED, and it was listed as a *foundation* of the plan. Wrong twice over: PR #51 rewrote that file on origin/main to use --update-env-vars + --update-secrets, and even the old file would not have overwritten anything, because gcloud raises ConfigurationError client-side when a literal collides with an existing secretKeyRef. The placeholder strings existed only in the stale local tree. Struck through in the table with the full explanation below it, and §2.3's warning block, §8's secret note, the Phase 0 blocker and the file list all rewritten. The conclusion "editing cloudbuild.yaml does not change production" survives — for the simpler reason that no trigger runs it. - H3's "401/402/429 leave no record at all" — narrowed. The platform request log does record them (timestamp/status/URL, 30-day retention); what is missing is attribution. The event is still worth building; its value proposition changes from "from nothing to something" to "from unattributable to attributable". - Two §0.4 open questions closed with measured values: _Default retention is 30 days, and Cloud Run has an empty cpu-throttling annotation (i.e. throttled), no minScale, maxScale=20, containerConcurrency=80. Added §0.6, a table of the statements the rest of this PR has already made obsolete, so a reader is not misled by prose describing the pre-fix code. Also redacted for a public repo: the local filesystem path to the source blueprint, and the other product's name in the comparisons — neither belongs in a public MIT repo. Paths in the doc are now repo-relative. Verified against: origin/main b868319 (2026-08-08). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both new migrations carried no instructions for how they actually reach the
production database, and the PR description said `prisma migrate deploy` —
which docs/agent-marketplace/HANDOFF.md §5/§6 explicitly rules out for this
repo: the `0_init` baseline directory sorts after every `0NN_` directory
('1' < '_'), so deploy would replay the whole history and end by re-running
the baseline, and the production `_prisma_migrations` table is documented as
an unreliable record of what is already applied. The documented procedure is
`prisma db execute --file <migration.sql>`, one file at a time.
Also records the deploy ordering each one needs, which differs:
- 010 is order-insensitive. `logRequest` catches its own write failure and
never throws, so whichever side is ahead only costs RequestLog rows.
- 011 must be applied BEFORE the new revision. `runHealthChecks` inserts the
history row in the same transaction as the Agent update, so if the code
ships first, every status flip in the gap rolls the whole transaction back:
the flip is lost AND `Agent.healthStatus` is left stale, counted only as a
writeError.
Comments only — no schema or statement changes. Both files were already
idempotent (IF EXISTS / IF NOT EXISTS), which is what makes the one-file-at-a-
time procedure safe to re-run.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ched catalog Security & ops (T0): - scripts/deploy-sync-job.sh no longer carries the production DB password; it binds DATABASE_URL from Secret Manager. The old literal must still be rotated. - scripts/check-secrets.mjs + a CI step fail the build on credential-shaped values in tracked files. - next.config.ts: HSTS, nosniff, frame-ancestors/X-Frame-Options, Referrer-Policy, Permissions-Policy, poweredByHeader off; CORS on the token-authenticated machine surfaces (/v1, /mcp, public catalog GETs). - Public API responses go through a whitelist (lib/agent-dto): reviewNote, publisherId and publisher avatars are no longer served. Correctness: - Home page, /api/agent and the weekly digest now filter status=APPROVED; pending and rejected skills used to appear in all three. - installCmd is only written for skills that exist on ClawHub, and migration 012 nulls the fabricated ones already in the table. - GET /api/skills/[id] no longer increments viewsCount; views are recorded by the page via POST .../view, deduplicated per visitor per day (lib/views). - /api/agent: case-insensitive search, pagination, cacheable. Performance: - lib/catalog: tag-cached catalog reads (home, scenario counts, categories) replacing nine uncached queries per home-page render. - Public catalog GETs send s-maxage + stale-while-revalidate. - Listings omit the README column. UX: - One search box with an agents/projects/skills scope (SiteSearch), replacing two boxes that went to different places. - Header: agents-first nav, a standing "Get API key" action, click-driven account menu with ARIA state, skip link. - Sign-in and sign-up carry callbackUrl, so gated pages return you where you were. - Branded 404, error and loading states for every locale route. - Profile is now "My listings": agents and skills together, with moderation status and the reviewer note (publishers previously had no way to see either). - Home page: value-first hero, a 30-second quickstart, health-filtered featured agents, only scenarios with real depth, one skills section instead of four. - Email templates speak the current product; added verification, password reset and review-outcome templates. Engineering: - Regenerated lockfile so npm ci works; .nvmrc and engines pin Node 20. - .github/workflows/ci.yml runs secrets, i18n, skill-asset, typecheck, lint, test and build. The old workflow only ran i18n and had been failing since 2026-08-08. - scripts/build-skill-assets.mjs generates the five copies of the skill body from takoapi_skill/SKILL.md, which had already drifted apart. - A shared Engagement component replaces the duplicated agent and skill bars. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- prisma/migrations/014: trigram GIN indexes on the columns every search filters with ILIKE '%q%', which no existing index could serve — each catalogue search was a sequential scan over 5.4k skills or 1.6k agents on a shared-core instance. Plus a GIN index on Agent.scenarios (migration 007 deliberately skipped it at ~500 rows; it now backs every scenario tile and filter) and the status+sort composites the listings actually order by. - Analytics: GA4 no longer loads on first paint. The site serves German, French, Spanish and Italian, so a real share of visitors are covered by the ePrivacy directive and there was neither notice nor a way to decline. Nothing loads until the visitor chooses; declining is one click and is remembered. - GET /api/skills/[id]/like now reports whether the viewer already liked it. The detail page assumed "not liked" on every load, so an already-liked visitor clicking the heart silently removed their like and showed a count one low. - /api/usage returns a 14-day daily series (grouped in Postgres, gaps filled) so the dashboard can show a trend instead of the last 15 rows. - authenticateApiKey throttles the lastUsedAt write to once a minute per key instead of once per gateway call. - withAdmin distinguishes 401 (not signed in) from 403 (signed in, not an admin); both used to be a 403 whose body said "Unauthorized". - /api/user/skills goes through the shared session-or-key resolver. - Contrast: text-gray-400 on real text fails WCAG AA on white; raised to gray-500 across the components not being rewritten elsewhere in this branch. - docs/06-cron-schedule.md: the new retention and ingestion-health jobs, the alerts that would have caught the two-month silent ingestion outage, and a key-expiry calendar (Apple's client secret expires 2026-09-25). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… the new shared code useAsync wrote to a ref during render (React 19 forbids it) — the caller already declares its dependencies through `deps`, so the ref was never needed. Analytics read localStorage into state from an effect; consent is external state, so it now goes through useSyncExternalStore, which also keeps the server and first client render agreed on "not answered yet". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e the pipeline's failures visible Sixteen tracks from the review, plus the fixes needed to make them build together. ## Discovery pages are server-rendered again /skills, /skills/[slug], /trending and /scenarios were client components that fetched after mount, so the entry points to 5,385 skills served crawlers an empty shell, and filters lived only in React state — unshareable, lost on refresh, dead to the back button. All four are now Server Components reading searchParams, with every control a link, real page numbers, and noindex on filtered permutations. /agents keeps its server rendering but trades a 40-chip wall for a filter sidebar, active-filter chips, and a sort default that depends on kind (calls for hosted agents, stars for projects) instead of likesCount, which is ~0 everywhere. ## The agent page answers "should I call this?" Multi-language call samples (curl, Python, TypeScript, A2A) from one shared source, a health badge with its last-checked time, and — for open-source entries — real GitHub metadata and a README excerpt behind a six-hour cache, so a project page is worth more than the repo link it used to be. The two verification badges that could never appear are gone. ## Publishing and moderation close the loop Publishers can validate an AgentCard before submitting (a preview of what will be listed), see field-level errors instead of one red bar, and watch status on /profile. Approving or rejecting now emails them with the reviewer's note. Admin mutations validate their bodies against the zod schemas that had been sitting unused, and invalidate the catalog cache so an approval shows up immediately. ## Auth Credentials login had no rate limit at all — /api/* is outside the proxy matcher and NextAuth owns the route, so password guessing was free. Now 8 attempts per 15 minutes per account+IP, checked before the user lookup. Password reset and email verification are built on the VerificationToken table, which had existed unused since the first migration; only token digests are stored. ## The gateway /v1/chat/completions honours stream:true (translating A2A SSE into OpenAI chunks, charging only a stream that completes), sends the whole message array instead of just the last user turn, and reports usage. /v1/agents/[slug]/message accepts full A2A messages with taskId/contextId so a multi-turn task can continue, and rejects an empty message before calling — and billing — the upstream. ## The pipeline stops failing silently The catalog's newest skill was dated 2026-07-04 while the sync job exited 0 every night: clawskills.sh changed its markup and the scraper matched nothing. Ingestion runs now emit a structured line, return non-2xx when a run found nothing at all, and a new /api/cron/ingestion-health reports per-source staleness — the signal that would have caught this on day one. ## Also - /docs: the API reference the funnel had no answer for. - Dashboard: onboarding checklist, named keys, a 14-day usage chart. - Admin screens rewritten off setState-in-effect, with in-page dialogs replacing prompt()/confirm() and DISABLED moderation exposed. - Sitemap excludes what the app itself marks noindex, so Google is not sent to pages that tell it to go away. - 158 unit tests, from zero: billing arithmetic and its deliberate fail-open, the AgentCard SSRF guards, scenario classification's word-boundary rules, callback-URL validation, pagination clamping, view dedupe. ## Build fixes found while integrating - <Analytics> sat outside NextIntlClientProvider in the root layout. Once the consent banner started calling useTranslations, that threw during prerender and broke the production build for every static page. Moved inside the provider; its server snapshot renders nothing, so the banner is never baked into static HTML. - InstallTabs.opencodeUse contained a literal <query>, which ICU parses as an unclosed tag and throws on. check:i18n never caught it because it skips ICU validation for keys whose English value does not parse. - check-secrets matched its own patterns and doc placeholders; the heuristics now recognise placeholder credentials while still catching real ones. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… does The reference omitted the kind filter, the hosted/project distinction that decides whether an agent is callable at all, streaming on the OpenAI shim, the rate limit and when a call is billed. Adds the check commands CI runs and the skill-asset generation step, which is easy to miss and silently drifts five files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
text-gray-400 on white is about 2.5:1 and fails WCAG AA. The copy button inside the dark command bar needed the opposite move — lighter, not darker — which a blanket replace would have got backwards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…link /docs
The English copy changed substantially in this branch — the positioning moved from
"OpenClaw skills marketplace" to "one API to access all agents", stale strings were
corrected, and 316 keys were added across the new pages. A key surviving does not
mean its translation survived: every value was re-checked against the current
English, not just the missing ones. Each catalog is now 961 keys, key-for-key with
en.json, ICU-valid.
Notable fixes that fell out of the pass:
- Plural categories now follow each language rather than copying English's
one/other. Russian gets few/many, Arabic its six forms; Chinese, Japanese,
Korean, Indonesian and Vietnamese drop the single-branch plural wrappers that
were only ever there to mirror English grammar.
- Admin.scenarioOption took a {nameZh} placeholder that no longer exists in the
English string. Any locale still passing it would have rendered a literal.
- Admin.scenariosEditorHint was half-Chinese in all fifteen files, including the
English one.
- InstallTabs.opencodeUse contained a bare <query>, which ICU reads as an
unclosed tag and throws on when the message is formatted.
/docs, added earlier in this branch, had no way in: it is now in the header (in
place of Blog, which stays in the footer) and in the footer's resources.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… Linux The lockfile regenerated earlier in this branch was resolved against an existing macOS node_modules, so npm recorded only the platform variants present there: CI on node:20 failed with '@emnapi/runtime, @emnapi/core, @swc/helpers missing from lock file'. Resolving from a clean tree records every platform's optional dependencies, and npm ci now succeeds on both npm 10/node 20 and npm 11/node 24. check-secrets: a value made of one repeated character is a shape, not a secret (the docs page shows tako_live_xxxxxxxx… as the key format). Verified against a random-looking key, which is still caught. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
Deploying thisCI is green. Beyond the usual deploy, this branch needs five migrations applied by hand, in order. for m in 010_requestlog_ip_hash 011_agent_health_history 012_installcmd_cleanup 013_password_reset 014_search_indexes; do
npx prisma db execute --url "$PROD_URL" --file "prisma/migrations/$m/migration.sql"
doneEvery file is idempotent, so a re-run is safe. Two need their ordering respected relative to the code deploy, and each explains why in its own header comment:
Environment
Scheduler
|
…e limits, lost charges
An 8-dimension review with 3 independent skeptics per finding raised 43 defects;
32 survived verification. This fixes all 4 high-severity ones and every medium.
## Authorization
- **A gateway key could act as an admin credential.** Routing the admin gate
through the shared `resolveApiUser` meant any `tako_live_…` key belonging to a
user with role=admin reached /api/admin/* — including role changes and the full
user list with emails. Those keys are meant to be handed out: the docs tell
users to paste one into an MCP client's config, and /api/keys mints them from
any session. The admin surface now requires a session, the legacy
single-purpose `User.apiKey`, or a key that explicitly names the `admin` scope
(nothing issues one). `hasScope` no longer treats an empty scope array as
granting `admin`, and has tests pinning that asymmetry.
- Skill submission uses the same resolver, and auto-approval stays with the
legacy admin key rather than following any admin's gateway key.
## Rate limits that were not limits
- The new credentials-login budget bucketed on `extractClientIp`, which reads the
first element of X-Forwarded-For — a value the caller writes, since Cloud Run
appends rather than replaces. One host could rotate the header and mint a fresh
8-try budget per request. Now keyed on the account alone, which an attacker
cannot rotate.
- /api/auth/forgot had the same hole, making it an open mail bomber pointed at
any address. Now 5/hour per target address.
- POST /api/skills/[id]/view was an unauthenticated write with no ceiling.
## Money
- **Streaming charges were issued after `controller.close()`** — that is, after
the response ends — which is the fire-and-forget hazard the rest of this branch
removed. Both streaming paths now settle before closing.
- **A call whose upstream body did not parse was billed as a success.** The
`.catch(() => null)` swallowed the failure, so status stayed 200 and errorCode
stayed null. The worst case is the 30s timeout landing mid-body-read: the caller
got HTTP 200 with empty content and paid for it, and a retrying client drained
its balance. Now a 502 with UPSTREAM_TIMEOUT / UPSTREAM_BAD_RESPONSE, billed
nothing, on both the OpenAI shim and the A2A route.
- The A2A stream billed a stream whose entire content was a JSON-RPC error, which
the shipped docs say is never charged. The relayed bytes are now observed (not
altered) so the failure is recorded and not billed.
## Privacy
- GET /api/skills/[id] served the moderator's `reviewNote` to anonymous callers
*and* sent it with a CDN-cacheable header, so one request could publish it to
everyone after. Now owner/admin only, on the uncached branch.
- The view dedupe key fell back to a hardcoded salt when TAKO_IP_SALT was unset —
making the digest reversible by anyone with the source — and accepted the "anon"
sentinel, which collapsed every such visitor into one key so only the first view
per skill per day counted. Both now skip the dedupe row and count the view.
## Correctness
- /api/admin/stats returned `pendingAgents` as a count while the dashboard read it
as rows, so the pending-agent review queue silently never rendered.
- The 14-day usage series ended yesterday, so the chart never showed the calls a
developer had just made.
- SkillEvent dedupe rows were written on every view and never swept;
`pruneSkillViewEvents` had no caller. Wired into the retention cron, with the
index the sweep needs.
- Registration mailed a "Confirm email" button pointing at /api/auth/verify, which
did not exist. It does now.
- Auth pages were wrapped entirely in `<Suspense fallback={null}>`, so the
prerendered HTML was an empty <main>. Split into a server page that awaits
`connection()` plus a client form, per the Next 16 docs.
- `noValidate` on the four auth forms turned a too-short password into "your reset
link is invalid".
## Accessibility and i18n
- Italian `Docs.mcpInstallNote`: an ICU apostrophe escape swallowed the <link> tag
and printed raw markup.
- The quickstart tab labels were English prose baked into a lib; they are keys now,
translated in all 15 catalogs.
- Two <SiteSearch> instances render at once and collided on the same DOM ids.
- CodeTabs' code panel is direction-locked and keyboard-scrollable, matching /docs.
- Directional arrows mirror in RTL; the last physical `ml-auto` is gone.
- Profile tabs are links, so they carry `aria-current`, not `role="tab"` with no
tabpanel; the health pill shows words rather than the raw enum.
172 tests (up from 158), tsc, eslint, i18n, secrets, skill-assets and the
production build all pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Arrows point along the reading direction. Five were still fixed left-to-right, so under dir=rtl the Arabic locale showed 'back' pointing forward and 'browse all' pointing backwards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
Adversarial review, and what it foundRan the branch through an 8-dimension review (compile, i18n, security, Next 16 semantics, data/migrations, billing, UX/a11y, cross-track consistency), with 3 independent skeptics per finding, each prompted to refute rather than confirm. 137 agents, 43 findings raised, 32 survived verification — 11 were refuted and dropped. All 32 are fixed in A gateway key could act as an admin credential. Routing the admin gate through the shared The login rate limit was not a rate limit. It bucketed on Streaming charges were issued after the response ended. A call whose upstream body did not parse was billed as a success. The All 32 confirmed findings
11 findings refuted by the verification passKept here because "we looked and it was fine" is worth recording: a type-check regression in 172 tests now (up from 158, and from 0 on |
oratis
added a commit
that referenced
this pull request
Sep 7, 2026
Both documents describe the state at 2026-09-05, and #54 has since implemented most of them. Without this header a reader meeting these files on main would take fixed problems for open ones — and would miss which two items code cannot close (the leaked database password and the Apple key expiry). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ass, stream billing A 10-dimension review of this branch (3 independent skeptics per finding, each prompted to refute rather than confirm) raised 36 findings and confirmed 20. These are the ones that bite in production; the rest were refuted or are notes. **The sync job could not start.** `daily-sync.ts` picked up an import of `../src/lib/ingestion-log` in 516144d, but `Dockerfile.sync` was never touched and copies only package*.json, prisma/ and the one script — so `npx tsx scripts/daily-sync.ts` died at MODULE_NOT_FOUND before main(). CI never builds that image, so nothing caught it. The irony is that the import exists to make the two-month silent ingestion outage impossible; shipping it this way would have frozen the catalogue again on the first nightly run. **An admin's gateway key could publish agents unreviewed.** Routing /api/agents/submit through `resolveApiUser` widened `via !== "session"` to include `via === "apikey"` — a `tako_live_…` key that /docs tells users to paste into MCP clients and CI. A leaked one could write an APPROVED Agent row with its own endpoint and price straight into the public catalog. The skills route had already been tightened; the two hand-written copies of the predicate had drifted, so both now call one `canBypassModeration()` with a test that fails if it is loosened. **Streaming charged for non-answers.** Neither streaming route checked the upstream content type, so an agent that does not implement `message/stream` and replies with a plain JSON-RPC body was relayed as a completed stream and billed in full — no `data:` lines means `frameIsRpcError`/`parseEvent` never run. Both routes now require `text/event-stream`, and a relay that ends without a single parseable event settles as NO_STREAM_CONTENT instead of as a success. The non-streaming shim had the matching hole: any parseable JSON without `result` was scored as a success, and an upstream 4xx/5xx was relayed to the caller as HTTP 200 with empty content. **Every response was a 200.** `[locale]/loading.tsx` put a Suspense boundary above every page in the subtree, and per Next 16.2's loading.md the status cannot change once the body streams — so `notFound()` returned 200 on all seven pages that call it, and a server error returned 200 too. With the database down the site would have reported itself healthy to every uptime check. Removed. **The secret scanner was blind to its own founding incident.** check-secrets exempts any line matching `@localhost/`, which is exactly the shape of the leaked Cloud SQL URL (`…@localhost/takoapi?host=/cloudsql/…`). Re-committing that password passed the scan. The exemption no longer applies when a unix-socket host override is present. **Anyone could lock any account out.** The login budget was per-account and spent before the password check, so 9 requests per 15 minutes against a known address held the owner out — indistinguishably from a wrong password, with /api/auth/forgot budgeted the same way. The budget now gates guessing, not the owner: over budget the password is still verified and a correct one still admitted; a separate, much wider ceiling bounds the bcrypt cost of a flood. Also: `{ id: "asc" }` tiebreakers on /skills and /api/skills, which were the only paginated listings left without one (rows repeat and vanish across pages); `Object.hasOwn` on two searchParams-driven lookups, where `?topup=__proto__` took the dashboard into the error boundary; and `aria-current` on the /skills filter chips, which signalled the applied filter by colour alone. tsc, eslint, vitest (176), check:i18n, check:secrets, check:skill-assets and next build all pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
oratis
added a commit
that referenced
this pull request
Sep 7, 2026
…-f13b3f UX + technical review documents (docs/07, docs/08) with phased optimization plans. The implementation landed in #54.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the review in #53. Also contains #52 (gateway/billing/privacy fixes) merged in, so #52 can be closed in favour of this one — or merged first, and this rebased.
170 files, +23.5k/−6k.
tsc,eslint,vitest(158 tests),check:i18n,check:secretsandnext buildall pass; CI now runs every one of them.scripts/deploy-sync-job.shcarriedpostgresql://postgres:…@…from 2026-07-04 in a public repo, anddocs/03-secret-hardening.mdrecords that step 5 (rotation) was never done. This PR removes the literal and binds the value from Secret Manager, but the password must be rotated and the history purged — removing it from the tip does nothing for anyone who already cloned.npm run check:secretsnow fails the build on any recurrence.APPLE_CLIENT_SECRETexpires 2026-09-25. After that, Apple sign-in fails silently — no error page, no log line. It needs re-signing (Apple caps the JWT at 6 months).docs/06-cron-schedule.mdnow carries a key-expiry calendar so this stops being tribal knowledge.What changed
The catalog is server-rendered again
/skills,/skills/[slug],/trendingand/scenarioswere client components that fetched after mount, so the entry points to 5,385 skills served crawlers an empty shell — no<h1>, no body text. Filters lived only in React state, so results could not be shared, did not survive a refresh, and the back button did nothing. All four are now Server Components readingsearchParams, every control is a link, pagination has real page numbers, and filtered permutations arenoindex./agentskeeps its server rendering but trades a 40-chip wall for a filter sidebar, active-filter chips you can remove, and a sort default that depends on kind — calls for hosted agents, stars for projects — instead oflikesCount, which is ~0 on every row and made "popular" a shuffle.The agent page answers "should I call this?"
Call samples in curl, Python, TypeScript and raw A2A from one shared source. A health badge with its last-checked time. For open-source entries, real GitHub metadata and a README excerpt behind a six-hour cache, so a project page is worth more than the repo link it used to be. The "Signed card" and "Verified publisher" badges are gone: nothing ever wrote those fields, so they could never appear.
Publishing and moderation close the loop
A publisher can validate an AgentCard before submitting and see a preview of what will be listed; errors land on the field that caused them instead of in one red bar. Submission status — including the reviewer's note — is now visible on
/profile, which lists agents and skills together. Approving or rejecting sends an email. Previously a publisher submitted into silence and had no way to learn the outcome.Admin mutations validate their bodies against the zod schemas that had been sitting unused in
src/lib/schemas.ts, and invalidate the catalog cache so an approval appears immediately.Auth
Credentials login had no rate limit at all —
/api/*is outside the proxy matcher and NextAuth owns the route, so password guessing was free. Now 8 attempts per 15 minutes per account+IP, checked before the user lookup so a flood costs neither a query nor a bcrypt compare. Password reset and email verification are built on theVerificationTokentable, which had existed unused since the first migration; only token digests are stored.The gateway
/v1/chat/completionshonoursstream: true(translating A2A SSE into OpenAI chunks, charging only a stream that runs to completion), sends the wholemessagesarray instead of just the last user turn, and reportsusage./v1/agents/{slug}/messageaccepts full A2A messages withtaskId/contextIdso a multi-turn task can continue, and rejects an empty message before calling — and billing — the upstream.The content pipeline stops failing silently
The catalog's newest skill was dated 2026-07-04 while the sync job exited 0 every night: clawskills.sh changed its markup and the scraper matched nothing. Ingestion runs now emit a structured log line, return non-2xx when a run found nothing at all, and a new
/api/cron/ingestion-healthreports per-source staleness — the signal that would have caught this on day one.Security and privacy
Security headers (HSTS,
nosniff,frame-ancestors,Referrer-Policy,Permissions-Policy),poweredByHeaderoff, and CORS scoped to the token-authenticated machine surfaces. Public API responses go through a whitelist:reviewNote,publisherIdand publisher avatars are no longer served. GA4 no longer loads on first paint — the site serves German, French, Spanish and Italian, and there was neither notice nor a way to decline.Performance
Tag-cached catalog reads replace nine uncached queries per home-page render; public GETs send
s-maxage+stale-while-revalidate; listings stop selecting the README column. Migration 014 adds trigram GIN indexes for theILIKE '%q%'searches that no index could serve — every catalog search was a sequential scan — plus the GIN index onAgent.scenariosthat migration 007 deferred at ~500 rows and now backs every scenario filter over 1,600.Correctness
/api/agentand the weekly digest now filterstatus = APPROVED. Pending and rejected skills appeared in all three.installCmdis only written for skills that exist on ClawHub; migration 012 nulls the fabricatedclawhub install <slug>commands already in the table, which failed for anyone who copied them.GET /api/skills/[id]no longer incrementsviewsCount— every crawler and prefetch was a "view". Views are recorded by the page, deduplicated per visitor per day.Engineering
158 unit tests, from zero: billing arithmetic and its deliberate fail-open, the AgentCard SSRF guards, scenario classification's word-boundary rules ("art" must not match "smart"), callback-URL validation, pagination clamping, view dedupe. CI runs secrets, i18n, skill-assets, typecheck, lint, test and build — the old workflow ran only i18n and had been failing since 2026-08-08, so nothing was being checked.
npm ciworks again.scripts/build-skill-assets.mjsgenerates the five copies of the skill body from one source; they had already drifted.i18n
All 14 non-English catalogs retranslated against the rewritten English rather than patched for missing keys — the positioning changed, so surviving keys had stale translations. Plural categories now follow each language (Russian gets few/many; Chinese, Japanese and Korean drop the single-branch wrappers that only mirrored English grammar).
Admin.scenarioOptionwas passing a{nameZh}placeholder the English string no longer has.Two bugs found while integrating
<Analytics>sat outsideNextIntlClientProvider. Once the consent banner calleduseTranslations, that threw during prerender and broke the production build for every static page. Moved inside the provider.InstallTabs.opencodeUsecontained a bare<query>, which ICU parses as an unclosed tag and throws on when formatted.check:i18nnever caught it because it skips ICU validation for keys whose English value does not parse — so the one broken English string was the one string it would not check.Follow-ups deliberately not in this PR
User.apiKeyis still stored in plaintext (the adminx-api-keypath).ApiKey.scopesandmonthlyQuotaare still not enforced. Both are called out in #53 and want their own change.🤖 Generated with Claude Code