chore: feat(seo): fold Google Analytics into the SEO report (lib/ga.sh) #331

Merged

joshtronic merged 1 commit from agent/329-feat-fold-google-analytics-into-the-seo-report into master

2026-07-04 01:24:14 +00:00

igor commented

2026-07-03 05:48:08 +00:00

Collaborator

What this PR does

feat(seo): fold Google Analytics into the SEO report (lib/ga.sh)
lib/ga.sh: ga_access_token, ga_property_for_domain (dynamic accountSummaries lookup, no static map), ga_run_report (GA4 Data API runReport, matches gsc_query's empty-rows error posture)
lib/seo-analysis.sh: new seo_ga_metrics parser; seo_build_report threads an optional ga field (defaults to null); both seo_render_markdown and seo_render_html add an "On-site behavior (GA)" section only when .ga != null
bin/tick.sh: sources lib/ga.sh; do_seo_tick resolves the GA property for the analyzed domain (after the zero-opportunity early-return, so a no-GA-property domain never makes a wasted GA API call) and merges GA metrics into the report before rendering
No new env knobs; agent-settings.json, systemd/, .forgejo/workflows/ untouched

Test plan

bash bin/test-ga.sh -- new suite: ga_property_for_domain (match/no-match/non-JSON/curl-failure), ga_run_report (request shape incl. empty-dims, happy parse, failure), seo_ga_metrics (full set, keyEvents-absent, empty-rows), seo_build_report ga passthrough, and both renderers' GA section (present with a property, absent/unchanged without one)
bash bin/check-sync.sh passes (full make test suite, all bin/test-*.sh green including the new one)
No manual verification needed beyond the above; this is a scripted, no-LLM code path with fixture-driven unit coverage

Closes #329

## What this PR does - [x] feat(seo): fold Google Analytics into the SEO report (lib/ga.sh) - [x] `lib/ga.sh`: `ga_access_token`, `ga_property_for_domain` (dynamic accountSummaries lookup, no static map), `ga_run_report` (GA4 Data API runReport, matches `gsc_query`'s empty-rows error posture) - [x] `lib/seo-analysis.sh`: new `seo_ga_metrics` parser; `seo_build_report` threads an optional `ga` field (defaults to `null`); both `seo_render_markdown` and `seo_render_html` add an "On-site behavior (GA)" section only when `.ga != null` - [x] `bin/tick.sh`: sources `lib/ga.sh`; `do_seo_tick` resolves the GA property for the analyzed domain (after the zero-opportunity early-return, so a no-GA-property domain never makes a wasted GA API call) and merges GA metrics into the report before rendering - [x] No new env knobs; `agent-settings.json`, `systemd/`, `.forgejo/workflows/` untouched ## Test plan - [x] `bash bin/test-ga.sh` -- new suite: `ga_property_for_domain` (match/no-match/non-JSON/curl-failure), `ga_run_report` (request shape incl. empty-dims, happy parse, failure), `seo_ga_metrics` (full set, keyEvents-absent, empty-rows), `seo_build_report` ga passthrough, and both renderers' GA section (present with a property, absent/unchanged without one) - [x] `bash bin/check-sync.sh` passes (full `make test` suite, all `bin/test-*.sh` green including the new one) - [x] No manual verification needed beyond the above; this is a scripted, no-LLM code path with fixture-driven unit coverage Closes #329

igor added 1 commit

2026-07-03 05:48:08 +00:00

chore: feat(seo): fold Google Analytics into the SEO report (lib/ga.sh)

Lint / check-sync (push) Successful in 5s

Details

Lint / check-sync (pull_request) Successful in 4s

Details

7405e0c044

igor commented

2026-07-03 05:51:12 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for 7405e0c0: success

Adds a GA4 client (lib/ga.sh), folds optional on-site-behavior metrics into the SEO report (lib/seo-analysis.sh), wires it into do_seo_tick, and ships a fixture-driven test suite. The change is well-scoped (~370 lines), the checklist matches the diff, CI is green, and the "additive/optional, degrade to GSC-only" design is clean. One integration point I can't verify from the diff keeps me from a full APPROVE.

The one thing to check before merging

lib/ga.sh:ga_access_token delegates to google_sa_access_token "$GA_SCOPE", but that function is defined in lib/google-auth.sh, which is not in this diff. The test suite stubs ga_access_token outright (bin/test-ga.sh:31), so nothing here exercises the real binding. If the shared minter is actually named something else, the feature silently no-ops in production (rc 1 → ga_property="" → GA section never renders) and every test still passes. Please confirm google_sa_access_token exists and takes a scope arg the way lib/gsc.sh uses it. This is safe-by-design (no breakage, just dead feature), which is why it's a comment and not a block.

Correctness notes (looked fine)

ga_property_for_domain returns empty+rc0 on no-match and empty+rc1 on fetch/parse failure; tick.sh:1704 handles both via || ga_property="" then the -n guard. Good.
ga_run_report and seo_ga_metrics both fall back to well-formed JSON ({"rows":[]} / null), so jq --argjson ga at tick.sh:1710 can't blow up on a bad fetch.
seo_ga_metrics transpose correctly drops keyEvents when the metric header is absent (covered by the NO_KEY_EVENTS test), and both renderers guard .ga.keyEvents != null.
GA call is placed after the zero-opportunity early return as the description claims — no wasted API call for no-GA domains.

Coverage gap worth a follow-up

Tests cover parsing, request shape, and both renderers thoroughly, but never the ga_access_token → google_sa_access_token seam (see above) nor do_seo_tick's end-to-end merge. Given the graceful-degradation design this is acceptable, but the real auth path is entirely untested.

No security concerns: domain is passed to jq via --arg, the property id interpolated into the runReport URL is the numeric API-supplied property field (not user/display input), and the bearer token isn't logged.

_{Independent review by the harness on claude-opus-4-8 (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `COMMENT` _(automated)_ CI for `7405e0c0`: **success** Adds a GA4 client (`lib/ga.sh`), folds optional on-site-behavior metrics into the SEO report (`lib/seo-analysis.sh`), wires it into `do_seo_tick`, and ships a fixture-driven test suite. The change is well-scoped (~370 lines), the checklist matches the diff, CI is green, and the "additive/optional, degrade to GSC-only" design is clean. One integration point I can't verify from the diff keeps me from a full APPROVE. **The one thing to check before merging** - `lib/ga.sh:ga_access_token` delegates to `google_sa_access_token "$GA_SCOPE"`, but that function is defined in `lib/google-auth.sh`, which is **not** in this diff. The test suite stubs `ga_access_token` outright (`bin/test-ga.sh:31`), so nothing here exercises the real binding. If the shared minter is actually named something else, the feature silently no-ops in production (rc 1 → `ga_property=""` → GA section never renders) and every test still passes. Please confirm `google_sa_access_token` exists and takes a scope arg the way `lib/gsc.sh` uses it. This is safe-by-design (no breakage, just dead feature), which is why it's a comment and not a block. **Correctness notes (looked fine)** - `ga_property_for_domain` returns empty+rc0 on no-match and empty+rc1 on fetch/parse failure; `tick.sh:1704` handles both via `|| ga_property=""` then the `-n` guard. Good. - `ga_run_report` and `seo_ga_metrics` both fall back to well-formed JSON (`{"rows":[]}` / `null`), so `jq --argjson ga` at `tick.sh:1710` can't blow up on a bad fetch. - `seo_ga_metrics` transpose correctly drops `keyEvents` when the metric header is absent (covered by the `NO_KEY_EVENTS` test), and both renderers guard `.ga.keyEvents != null`. - GA call is placed after the zero-opportunity early return as the description claims — no wasted API call for no-GA domains. **Coverage gap worth a follow-up** - Tests cover parsing, request shape, and both renderers thoroughly, but never the `ga_access_token → google_sa_access_token` seam (see above) nor `do_seo_tick`'s end-to-end merge. Given the graceful-degradation design this is acceptable, but the real auth path is entirely untested. No security concerns: `domain` is passed to jq via `--arg`, the property id interpolated into the runReport URL is the numeric API-supplied `property` field (not user/display input), and the bearer token isn't logged. --- <sub>Independent review by the harness on `claude-opus-4-8` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-07-03 05:51:12 +00:00

53 seconds

igor requested review from joshtronic

2026-07-03 05:51:13 +00:00