Fix GSC row limits, aggregation semantics, paid-call retries and SSRF - #10
Merged
Conversation
Three read-only Codex passes over the repo returned ~60 findings that collapse into about eight root causes. Phase 1 takes the ones that make the app print a wrong number or spend money. Five claims were verified by hand before writing. The cross-dialect timestamp finding turned out to be schema-wide rather than a single bad comparison, and the parity test that should have caught it compares hasDefault as a boolean while its name claims it checks defaults. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Covers workstreams 1.1 and 1.2 of the Phase 1 spec; they share a plan because the aggregates consume the truncation flags. Reading the code shrank the work. The truncation-honesty machinery is already written -- trendingOpportunities sets QUERY_ROW_LIMIT = 5000 and computes currentTruncated so it "says so rather than pretending" -- but buildSearchAnalyticsRequest clamps every caller to the MCP path's 1000, so the flag can never fire. Making the ceiling a parameter unblocks it. Task 5 is flagged for sign-off rather than treated as a bugfix: the min-position behaviour is a documented deliberate choice, and only its implementation is wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The 1000-row cap protects the MCP agent's context window. The analytics UI inherited it silently, which made downstream truncation flags unreachable: callers requested 5000, received 1000, then tested rows.length >= 5000 and always concluded "not truncated". The ceiling is now a parameter. MCP keeps 1000; analytics gets 5000. GSC_MAX_ROW_LIMIT had exactly one consumer -- the MCP tool schema, which wants the MCP ceiling -- so it is renamed rather than aliased. Also corrects a comment claiming 1000 was "GSC's per-call max". GSC allows 25000 per request plus startRow pagination. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Measures the real aggregation functions rather than a reimplementation, and counts JSON.parse of the provider response -- which turns out to dominate, so justifying the ceiling on aggregation alone would have been misleading. rows payload parse aggregate total 1000 0.1 MB 0.33ms 0.51ms 0.84ms 5000 0.7 MB 1.83ms 1.55ms 3.38ms 10000 1.3 MB 4.12ms 2.22ms 6.35ms 25000 3.3 MB 8.76ms 6.91ms 15.67ms Workers Free allows 10ms CPU per invocation, shared with routing, auth, D1 and serialization. 5000 stands; 25000 is not reachable in-request regardless of what the GSC API permits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fetchAllRows returns { rows, rowsExamined, truncated } so callers can
tell "nothing exists" from "nothing in the rows we fetched".
Verified against Google's live docs before writing it: max rowLimit is
25000, rows are guaranteed clicks-descending, startRow past the end
returns zero rows, and a short page is an explicitly documented
exhaustion signal ("if you get less than the number of rows requested,
you have retrieved all the data").
Typically makes ONE request -- our ceiling is below the provider maximum,
so there is nothing to paginate. The loop only matters if a caller passes
a smaller rowLimit. Documented the two limits it cannot overcome: GSC
exposes only top rows, and click-tied rows have arbitrary order across
requests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit made the ceiling a parameter but nothing passed it,
so every app path was still clamped to 1000. Codex caught this: the app
goes through GscService.getPerformance, which called the request builder
with no ceiling.
Adds GscService.getAnalyticsPerformance and moves all 14 app call sites
to it. The MCP call site keeps getPerformance. It is a separate name
rather than an argument so an unmigrated call site shows up in a grep
instead of being silently clamped; the default stays the SMALL ceiling so
forgetting to choose cannot flood the agent's context.
Truncation now compares against request.rowLimit -- the limit actually
applied -- instead of a local constant. Those diverged silently before,
which is why currentTruncated was permanently false.
QUERY_ROW_LIMIT drops 5000 -> 2500 on measurement, not preference. That
handler parses THREE payloads per request:
rowLimit x3 payloads size parse + momentum
1000 0.3 MB 1.49 ms
2500 0.8 MB 3.65 ms
5000 1.7 MB 7.71 ms too tight
Still 2.5x more rows than it was really getting, inside a 10ms budget.
The ceiling is a ceiling, not a target -- an endpoint parsing three
payloads cannot spend all of it.
Exports now paginate via fetchAllRows to the ceiling and return
rowsExamined/truncated, instead of taking one 1000-row page and calling
it the full dataset. GSC orders by clicks descending, so the silent cut
dropped exactly the long-tail rows a spreadsheet user is hunting for.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
buildQueryTotals summed clicks and impressions across query x page rows and called the result the query's totals. Google counts a property once per impression however many of its URLs appear, while page rows count each displayed URL, so a property showing two URLs for one query reported two impressions instead of one. Branded/non-branded splits and every ranking list built on those totals inherited the inflation. Query totals now come from their own dimensions:["query"] pull. Ordering and the 500-row cap are preserved deliberately, so this moves the numbers without also reshuffling which queries appear. Sets aggregationType:"byProperty" explicitly. Codex found we never set it at all, so we were getting "auto" -- Google decides -- and the client discarded responseAggregationType, so we could not even tell what we got. A silent byPage response would have reintroduced the double count, which makes that field load-bearing rather than cosmetic. It is now returned and typed. Adds gscAggregation as the single owner of GSC row semantics, with representativePageForQuery replacing MIN(page average position) -- a value Google defines no metric for, and which let a page with one impression at position 1.0 outrank a page with 1000 impressions at position 8.0. Wiring the remaining min-position call sites is next. Drops two tests that encoded both bugs as expected behaviour, including one named "keeps the best page's position". Their replacements in gscAggregation.test.ts assert the property-vs-page rule directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Striking distance and internal-link targeting both collapsed a query to its lowest-average-position page. That let a page averaging position 1.0 off a single impression beat a page averaging 8.0 off a thousand, so: - striking distance judged the query already-ranking and dropped it, hiding the real opportunity on the only page anyone sees - internal-link recommendations pointed at a URL nobody reaches Google defines no metric equal to MIN(page average position). Position is averaged over impressions per row, so a minimum across separately averaged rows reconstructs nothing. Link opportunities now also skip queries no page owns (<60% impression share), because there the right action may be consolidation rather than more links toward an arbitrary winner. The two existing striking-distance tests passed unchanged through this rewrite -- their position-leader and impression-leader happened to be the same page, so neither could distinguish the implementations. Added the divergent case, which fails against the old code (it returned zero rows), plus one confirming the band filter still applies to the owning page. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A row at position 8 with high impressions and no clicks legitimately appears in both striking distance and CTR opportunities. The two loops appended independently, so the same query and page produced two entries whose estimates were then added: an 80-click quick win plus a 30-click title rewrite displayed as "110 clicks at stake if all are fixed". Those are two descriptions of the same impressions reaching the top three, not independent gains. Moving the row into the top three already subsumes part or all of the rewrite gain. Opportunities are now keyed by query and page. Overlaps take the larger scenario, the kind follows the larger estimate so the headline action is the bigger win, and both signals stay named in the detail so the merge is visible rather than silent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three empty states asserted absence from a clicks-ordered pull that Search Console does not promise is complete: - Cannibalization: "No cannibalization detected -- that's a healthy site" - Link Opportunities: "No opportunities right now" - Trending Opportunities: "Search Console has no queries with enough impressions yet" The link-insights server function returned no truncation indicator at all, so the UI could not have told the difference; it now returns rowsExamined and truncated, derived from the applied row limit. Trending Opportunities already had well-written truncation notices -- on the SUCCESS path only. The empty path, where a false all-clear actually misleads, had none. It does now. Each keeps its original confident wording for the case where the pull really was exhausted, because that claim is true then. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
getSearchPerformanceReport and getContentPerformance now return a
sampling flag. One plumbing change covers four features: Search
Performance, Dashboard, SEO Opportunities and the shared suggestion
chips all read the same report.
Conditional now, where they previously asserted absence outright:
- "No striking-distance queries in this period"
- "No CTR laggards found -- every well-ranking query is earning a
healthy share of clicks"
- "No search queries yet in this period"
- "No near-miss queries -- everything is already top 3 or far off"
- "No keyword opportunities right now"
- "No content groups in this period"
Deliberately NOT following the review's suggestion to suffix ~15 counts
with "in fetched sample" ("Branded in fetched sample", "Sample page 3 of
5"). The counts are honest -- they are what we found. Fifteen sampling
tags would make the product read as broken. One notice per panel says the
same true thing without nagging.
Content Performance gets its own note: both periods are independently
capped to their top pages by clicks, so a page can leave the sample
without leaving the site, and the comparison can show movement the real
data does not have.
Two superlatives corrected rather than qualified, because after the
aggregation fix they describe the old semantics: the suggestion chip's
"best page ranks #N" and the dashboard's "best position" now read as
plain rank. Those rows carry Google's property-level average position for
the query and identify no page at all.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fetchAllRows, describeSampling and link-insights each had their own copy of "did this pull come back full". Three implementations of the predicate that decides whether we may tell a user something does not exist is the same drift this branch exists to remove. pullWasTruncated is now the single definition, and its doc states the rule that was actually broken: compare against the limit the request APPLIED, never the limit the caller asked for. Tested directly, including the conservative no-recorded-limit case -- absent a known limit we assume truncation, because over-claiming absence is the failure mode that matters. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Site Audit was the worst of these. "This crawl found N issue types, but
none of them touch your highest-traffic pages -- nothing urgent to fix
there" is an all-clear derived from at most the 20 top-clicked pages GSC
returned. Issues on page 21 downward were never compared. It now names
the number of pages actually checked and says the rest were not.
Two adjacent overclaims in the same verdict: "N of your top-clicked
pages" gave no scope, and "(N affected sitewide)" described pages found
in ONE CRAWL, which is only the whole site if the crawl was exhaustive.
Also conditional now: Local SEO's three "no branded queries / no local
landing pages / no results appeared" states, and the topic-coverage
tooltip that read "No matching Search Console landing page" for a topic
that may simply rank below where the pull stopped.
Trends' "Covers the whole property" was plainly false -- it covers a
capped pull. Rephrased to what it was actually there to say: the location
picker applies to the chart, not the list.
On-Page's "did not produce any fixes" now notes that query-informed title
suggestions depend on capped Search Console rows.
Not done, deliberately: the review also proposed relabelling ~15 counts
as "(generated)" or "shown" ("Approve all 5 shown", "All generated (12)").
Those counts are accurate descriptions of what is on screen; the suffixes
would add noise without correcting a false claim.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
getSearchPerformanceReport gained a query-dimension pull for property totals, taking it to five concurrent pulls. Measured rather than assumed: all five payloads together (200+200+1000+2500+25 rows, 0.32 MB) parse and aggregate in ~3.0ms median, inside the 10ms Workers Free budget. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex was briefed to refute the branch. Five real defects, three of them introduced by my own fixes. 1. Striking distance lost real opportunities (regression I introduced). Collapsing each query to its impression LEADER before the band check discarded a page holding 40% of a query's impressions at position 8 because a bigger page ranked 35th. The reconciliation: the ORIGINAL rule was right and its INPUT was wrong. Take the best-positioned page among those carrying meaningful impressions (>=5% share). That keeps the feature's premise -- already ranking above the band means improving a secondary page won't move traffic -- while refusing to let a one-impression fluke stand in for "the site already ranks". 2. Consolidation was merged into the wrong action. Merging by query+page swallowed cannibalization rows, and `kind` drives the badge AND the CTA, so users were sent to "Build brief" for work that is about redirecting a competing URL. Only striking-distance and CTR overlap now merges; consolidation stays its own task. 3. Exports could duplicate and skip rows. I documented that click-tied rows have arbitrary order across requests, then set the export page size to 1000 anyway -- five paginated requests, each able to straddle a tie. Now one request, since the ceiling is below GSC's 25000 limit. 4. Truncated exports still looked complete. The server returned truncated/rowsExamined and the client discarded both. A spreadsheet outlives the screen it came from, so it now warns on export. 5. Sampling metadata described the wrong pull. Combining several pulls' flags while reporting one pull's count let the UI say Search Console "returned 700 query-and-page rows and stopped there" when that pull finished early and a larger one hit its limit. Now per-pull, and each consumer reads the source its own claim rests on. Plus guards on fetchAllRows: a zero rowLimit looped forever requesting nothing, and a provider returning more rows than asked for blew through the ceiling that exists to bound CPU. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
"No urgent issues detected" is the highest-stakes absence claim in the product -- it goes to a client who may act on it for a month -- and it was the one place the truncation sweep missed. It now says what was retrieved when either GSC source hit its limit. Also records the CPU measurement that did NOT go my way. Adversarial review argued row count is a proxy for CPU rather than a bound, and it is right: at realistic key lengths the three trending-opportunities payloads are 0.6-1.7 MB and run 3.3-4.5ms, but with ~200-char queries and ~1500-char URLs the same 2,500 rows produce 5.8 MB, measuring 6.97ms median and 11.30ms worst -- over the 10ms budget, on a dev machine. That shape is implausible across 2,500 rows but not impossible, so the limitation is written next to the constant instead of only the favourable figures. If this endpoint ever throws Cloudflare 1102, check payload bytes before row counts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
buildPropertyQueryTotals caps at 500 rows while the pull now returns up to 2500, so "Ranking queries: 500" silently plateaus and reads as a total. Kept the cap rather than raising it: every consumer already slices to six rows or five suggestions, so the extra 2000 rows would only grow the wire payload and its serialization CPU -- spent on the same invocation as parsing, already measured near half the Workers Free budget for this endpoint. Trading a measured constraint for a cosmetic count is the wrong way round. Instead the tiles now say what they counted, and the constant documents that its length is a display slice while sampling.queryTotals.rowsExamined is the real figure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two date defects, both confirmed against Google's live docs. subtractRange subtracted a full N from an inclusive end date, so "last 28 days" covered 29 dates and "last 7 days" covered 8. Every headline period comparison ran on a window one day longer than its own label, which also shifted its weekday mix against the previous period. Now N-1. Month-named ranges keep calendar-month arithmetic and are unchanged. "Today" was derived in UTC. Google interprets startDate and endDate in Pacific Time, so for the seven or eight hours between Pacific midnight and UTC midnight every convenience range was a day ahead of the calendar Google reads them against. resolveDateRange now converts the instant to the Pacific calendar date first. The old test pinned the off-by-one as correct -- it asserted 2026-04-27 to 2026-05-25, which is 29 dates for a 28-day range. Replaced with an inclusive day COUNT, so the bug cannot return wearing a different literal, plus a case either side of Pacific midnight. Two 16-month-floor expectations shifted a day for the same reason and are updated with that reason recorded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
DataForSEO's live and task_post endpoints are POSTs billed per request. core.ts retried any 5xx twice while its own comment called the operation "an idempotent read" -- it never checked the method. A 5xx does not tell us whether the provider already did the work and charged for it, so a replay can buy the same result again. Retries are now GET-only, via an exported shouldRetryDataforseoRequest so the money rule is directly testable. core.ts had no test at all; it now has five, including that an absent method defaults to NOT retrying. Three layers stacked on top of that transport: - lighthouse.ts looped three times per strategy. With the transport's two retries beneath, one strategy could issue nine billable calls. Now one attempt; a failed page is recorded as failed, which AuditRepository already persists honestly with null scores. - The lighthouse-batch workflow step ran with no config, so Cloudflare's default retry policy applied to 10 URLs x 2 strategies = 20 billed calls. If the provider calls succeeded and an R2 upload failed afterwards, the whole batch replayed and re-bought all 20. Now zero-retry, matching the convention siteAuditWorkflowFallback.ts already documents for the same reason. - Four client queries on paid paths used retry: 1, doubling server function invocations that each reach the provider independently. Also corrected two log lines still claiming "after 3 attempts". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Following redirects means letting a remote server choose which address our Worker connects to, so it cannot be delegated to fetch(). All three places doing it validated only the URL the user submitted, or nothing at all. fetchValidatingEveryHop follows redirects manually, re-runs the full url-policy check on each hop, optionally pins every hop to one host, and refuses chains over five hops rather than returning a half-followed one. The reported instance, link-insights checkLinkPresence: an authenticated project member could submit a page they control that answers 302 Location: http://127.0.0.1:8787/..., and the Worker made that request -- straight past the private-address protections in url-policy.ts. Two more of the same class, found while fixing it: - siteTextCrawl.fetchHtml was worse: it builds https://${domain} from user input and had NO url-policy check at any point, so a domain of "127.0.0.1:8787" was fetched directly. - crawlPage checked isSameOrigin AFTER the fetch, so a crawled page redirecting to a private address had already been requested by the time the result was discarded -- enough to probe internal services. url-policy.test.ts gains six hop tests: loopback, private range, off-host, a legitimate same-host redirect, an endless loop, and that redirect handling is always manual. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four claims the underlying data cannot establish, plus one piece of
guidance stated as a diagnosis.
Backlink profile verdict: two branches widened from the page to the whole
profile ("this profile is built almost entirely from low-authority
sites"). It is computed from one 100-row page; with totalCount at 1000 the
unseen 900 can invert the verdict either way. Every sentence is now
page-scoped, which the other two branches already were.
Nofollow exposure: dropped the inference that the headline referring-domain
count "overstates how much authority reaches this site". DataForSEO's
referring_domains_nofollow counts domains with AT LEAST ONE nofollow link,
so if all of them also send a followed link -- which this field cannot
rule out -- every domain passes authority and nothing is overstated.
Domain keyword verdict: dropped "no single ranking loss should sink this
domain's traffic". The input is keyword counts per position band plus one
aggregate traffic total; 35% of keywords on page one is entirely
compatible with one of them carrying 99% of the traffic. It now reports
ranking breadth and says where the answer actually lives.
Cannibalization subtitle: asserted pages "compete against each other,
splitting clicks and rankings" and went straight to "consolidate". The
request dimensions are query and page only -- no device, country, date or
same-SERP coexistence -- so we observe plurality, not competition. States
the observation, then the likely reading, then says to check before
merging, because the recommended action destroys a page.
CTR opportunities: kept as guidance but softened from a diagnosis. GSC
reports clicks, impressions and position, never why someone didn't click;
a featured snippet or brand preference produces the identical row and a
title rewrite fixes neither.
Two tests asserted the removed phrasing and now assert the new contract,
including that the nofollow note never claims authority is overstated.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The two schemas store timestamps as text in DIFFERENT formats and the app compares them as strings: D1 sql`(current_timestamp)` -> "2026-07-22 10:00:00" Postgres isoNow -> "2026-07-22T10:00:00.000Z" Each sorts correctly alone. Mixing them does not: "T" (0x54) sorts above a space (0x20), so a SQLite-shaped cutoff compared against ISO rows excludes every row on the cutoff date whatever its time. On Postgres a seven-day rank comparison silently read an older snapshot and reported the wrong delta -- a wrong number, not an error. toStoredTimestamp formats for the active provider; both comparison call sites use it. Chose this over the spec's plan of migrating D1's stored values to ISO: the migration is the tidier end state but rewrites live rank-tracking history in production to fix a defect that only ever affected the Postgres path, which nothing currently runs on. Reformatting the comparison is behaviour-preserving on D1 and correct on Postgres. Also fixed the guard that should have caught this. schema-parity's assertion is named "(name, nullability, type, default, enum)" but records hasDefault as a BOOLEAN -- it only ever checked that both sides have a default, never what it produces, which is how the two databases came to hold incompatible values with the suite green. It now asserts the known divergence per column, with a suite-level check that the detector still finds real columns so it cannot go vacuous. And the same-second tie: the latest-snapshot join matches on timestamp equality, and D1's current_timestamp has one-second precision, so two runs in the same second both matched and the consumer's last write won in database order. The displayed rank could flap between 5 and 10 on identical data. Ordering is now stable. Deletes snapshotQueries.test.ts: its one assertion is duplicated in rankTrackingTimestamps.test.ts with better context, and the filename promised snapshot-query coverage it never contained. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex reviewed the 1.3-1.7 work briefed to refute it. Nine findings; the three highest severity were regressions from my own fixes. 1. A transient R2 failure now failed the WHOLE paid audit. Setting the lighthouse-batch step to zero retries was right -- it stops a replay re-buying 20 billed calls -- but fetchAndStoreLighthouseResult threw on R2 upload failure AFTER the provider calls were billed, so one 503 rejected Promise.all and failed the audit. Two concerns were conflated: the paid call must not be replayed; storing its output is free and independently recoverable. Upload failures now keep the scores and lose only the payload link. 2. The www -> apex canonical redirect broke audits. My hop check pinned the exact hostname, but isSameOrigin deliberately treats apex and www as equivalent -- so a normal 301 failed, crawlPage returned a status-0 page, and with allPages.length === 1 the DataForSEO fallback was skipped too. An audit could finish with one failed page. 3. Cross-port SSRF was still open. Comparing only hostname let a redirect to http://same-host:8080/admin through, and the Workers runtime can subrequest custom ports -- turning the phrase check into a content oracle against another service on that machine. 2 and 3 share one fix: fetchValidatingEveryHop now takes an `allowHop` PREDICATE instead of a hostname, so callers pass their own origin rule and this module keeps owning SSRF policy alone. All three callers pass isSameOrigin, which checks hostname equivalence, protocol and effective port. 4. Manual redirect following silently erased redirect metadata: response.redirected is always false when every hop is a separate manual fetch, so every recorded redirectUrl became null. The helper now returns finalUrl and redirected. 5. The client report still declared all-clear when GSC was MISSING, not just capped -- a failed or disconnected request became null, counts fell to zero, and every branch went quiet. gscSampled is now gscIncomplete and covers absent data. Missing data is not data showing nothing. 6. Cannibalization: I softened the subtitle and left "winner", "competing pages", "consolidate first" and "% off the leader" in the badges and rows -- half-corrected reads worse than untouched. The uncertainty now runs through every label, here and in the client report. 7. Month-named ranges had two defects I chose to leave alone. setUTCMonth OVERFLOWS: "last 3 months" from 2026-05-31 started 2026-03-03, dropping March 1-2, and 12 months spanned 366 inclusive dates. Now exclusive-end subtraction with day clamping, shared with the 16-month floor which had the same overflow. 8. The sibling domain verdict still asserted traffic "concentrates in that thin slice" from position-band counts -- the exact claim I had just removed from the branch next to it, and it fired even with zero page-one keywords. 9. DR wording still inferred link weight and called a mix normal or unusual. DR describes the referring domain's own profile, not the placement or follow status of the link pointing here, and "unusual" needs a niche benchmark we do not have. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…he browser Third Codex pass on this branch. Six findings -- down from 10 then 9 -- and the date arithmetic came back clean across five worked examples. Three were regressions from my own previous round of fixes: 1. The R2 catch swapped a terminal failure for a silent one. Scores were kept but the row became indistinguishable from a success: progress counted it completed, the payload was lost, and the comment claimed storage was "independently recoverable" when nothing recovered it. Uploading is FREE, so it is now retried 3x inside the non-replayable paid step, and the residual gap -- a scores-but-no-r2Key row looks like an old no-payload row, visible only in logs -- is written down rather than implied away. Closing it properly needs a payloadStatus column. 2. "% of traffic outside the leading page" measured a different page from the one wearing the trophy: isWinner sorts by POSITION, splitShare measures clicks outside the CLICK leader. My rewording verbally merged two incompatible definitions. Now "% of clicks outside the top-clicked page" and a "best rank" badge that says it is not necessarily the click leader. 3. gscIncomplete conflated PENDING with settled-and-missing, so printing the report mid-load asserted "that data was incomplete" about requests still in flight. Pending is now a third state with its own sentence; excluding it alone would have emitted the confident all-clear instead. Two were pre-existing bugs I had newly started depending on: 4. areEquivalentHostnames gave a www. origin THREE accepted hosts: the prefix tests let www.www.example.com satisfy `a === "www." + b`. A redirect there counted as inside the audit boundary. Now canonicalizes by stripping at most one www. from each side. 5. Every hop was compared against the ORIGINAL origin, so an http chain that upgraded to https could hop back to http and still match. Transport security is a property of the chain, so fetchValidatingEveryHop now makes https sticky. One was my own spec item I had not implemented: 6. checkLinkPresence matched source against target only -- self-authorizing, since both come from the submitter. Any member could use mentionsPhrase as an oracle against an unrelated public site, varying phrase to miss the cache. Both URLs and every hop are now pinned to the project's own origin, which is what the spec asked for. And one I found by loading the app: the Opportunities tiles rendered a confident "0 opportunities / 0 clicks at stake" with GSC unconfigured, because a disconnected source is not an isError -- the query SUCCEEDS with connected: false. Extracted isSourceUnavailable (tested), which also covers pending. Browser-verified 0 -> "—", with the audit-derived tiles correctly still showing 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
flyrocketseo | 6b2b4ff | Jul 30 2026, 03:04 AM |
This was referenced Jul 30, 2026
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
1 participant
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.
Phase 1 of the overhaul: everything that made the app print a wrong number or spend money it shouldn't.
Spec:
docs/superpowers/specs/2026-07-29-correctness-and-money-overhaul-design.mdPlan:
docs/superpowers/plans/2026-07-29-gsc-data-correctness.mdpnpm ci:checkgreen · 2,087 tests passing · 22 commits · 67 filesWhy this branch exists
Three read-only Codex passes over the repo returned ~60 findings. They collapsed into about eight root causes, so this fixes causes rather than symptoms — one change to the GSC row cap resolved six separate user-visible defects.
What was actually wrong
The row cap.
GSC_MAX_ROW_LIMIT = 1000exists to protect the MCP agent's context window. The analytics UI silently inherited it, so callers asked for 5,000 rows, received 1,000, then testedrows.length >= 5000to detect truncation — a condition that could never be true. Every "no opportunities found" was computed from a clicks-ordered sample while reporting itself complete.Double-counted demand.
buildQueryTotalssummed["query","page"]rows as if they were property totals. Google counts a property once per impression however many of its URLs appear; page rows count each URL. A property showing two URLs for one query reported two impressions. That inflation flowed into branded/non-branded splits, cannibalization severity and every clicks-at-stake figure.Position semantics.
MIN(page average position)let a page with 1 impression at position 1.0 outrank a page with 1,000 impressions at 8.0 — hiding real striking-distance work and pointing internal links at URLs nobody reaches. Google defines no metric equal to that minimum.Money.
core.tsretried any 5xx twice while its own comment called the operation "an idempotent read" — it never checked the method, and DataForSEO's billable endpoints are POSTs. Stacked on top: Lighthouse looped 3x per strategy, and thelighthouse-batchworkflow step ran with no config, so Cloudflare's default retries applied to 20 billed calls and an R2 failure after the provider succeeded re-bought all twenty.SSRF, three instances.
checkLinkPresencefollowed redirects behind an origin-only hostname check. Looking for others found two worse:siteTextCrawlbuilthttps://${domain}from user input with no policy check at all, andcrawlPagechecked same-origin after the fetch — so the request to a private address had already happened.Dates. "Last 28 days" spanned 29 dates, "last 7" spanned 8, and "today" resolved in UTC when Google reads these in Pacific. Month ranges overflowed:
2026-05-31minus 3 months landed on March 3, dropping March 1–2. The existing test asserted the off-by-one as correct; it now counts inclusive days.Cross-dialect timestamps. D1 and Postgres store timestamps as text in different formats (
2026-07-22 10:00:00vs2026-07-22T10:00:00.000Z)."T"sorts above a space, so a SQLite-shaped cutoff excluded every ISO row on the cutoff date and Postgres rank deltas silently read the wrong snapshot.Absence claims. Roughly a dozen strings asserted things the data couldn't establish — most starkly "No cannibalization detected — that's a healthy site" from rows we never fetched, and a Site Audit all-clear computed against at most 20 pages.
Review notes
Three adversarial Codex passes ran against this branch (10 → 9 → 6 findings). Each round found regressions introduced by the previous round's fixes, so the fix commits are worth reading as carefully as the original work. Notable self-inflicted ones, all fixed:
www→ apex, which could end an audit with one failed page and skip the fallback.Deliberately not done
payloadStatuscolumn. A Lighthouse row with scores but nor2Keyis indistinguishable from an older no-payload row, so an upload failure is visible only in logs. Closing that needs a migration.What is NOT verified — please check before merge
No truncation state has ever been rendered. GSC is unavailable locally ("Google OAuth client not configured"), so every
truncated-conditional string is verified by logic and tests only. This needs a property with enough queries to actually truncate.Browser verification did happen and is worth knowing what it covered: every changed page renders against a real project with zero console and zero server errors, which closes the runtime shape-mismatch risk from changing server-function return types. It also found a bug tests missed — Opportunities rendered a confident "0 opportunities / 0 clicks at stake" with GSC disconnected, because a disconnected source isn't an
isError.Two visible behaviour changes
🤖 Generated with Claude Code