Code Review: PR #369

Commit: ec31efa98f9b46c25b4daf9264341725a3774f3f

Summary

Ships 3 of 4 GEO-audit polish items from #333: agent-permissions.json (item B), Copy-for-AI button on 3 marketing pages (item D), and llm:token-count + llm:doc-length meta tags + a drift smoke test (item C). Item A (ai-plugin.json) is explicitly skipped with a documented rationale in the PR body. Scope is tight — 6 files touched, all under site/ except the smoke-test extension which is the AC-required test.

Checklist Results

• Architecture & Design: Pass (no domain/application/infra concerns — static site assets)
• Code Quality: Pass (vanilla ES2017 with no any, IIFE-wrapped, strict-mode JS; well-commented bash test)
• Testing: Pass (smoke test re-run locally: all 3 pages within 5%; verified failure modes — missing meta exits 1, drift > 5% exits 1)
• Security: Pass (window.open(url, "_blank", "noopener") is correct form; credentials: "omit" on fetch; no inline secrets)
• Performance: Pass (defer-loaded JS, no blocking parse; negligible payload)
• PR Description & Glossary: Pass (5-term glossary, narrative summary bullets, decisions documented inline)
• Summary Bullet Narrative: Pass (each bullet answers what + why)
• Technical Decisions (AgDR): N/A (no new libraries, frameworks, or architecture patterns — agent-permissions.json schema choice is a one-off GEO-audit polish, not a portfolio-wide tech call)
• Adopter Handbooks: N/A (no findings — handbooks loaded but none apply to static site assets)

Verification performed locally at HEAD ec31efa

Issues Found

None blocking.

Suggestions (non-blocking)

1. Latent zero-divide error in the drift check (test_site_counts.sh:236-237, :239-240). The bash ternary actual_chars > 0 ? diff_chars * 100 / actual_chars : 0 evaluates both branches in arithmetic context before short-circuiting. If a file is 0 bytes, you get a division by 0 stderr message and pct_chars ends up empty (which the -gt 5 then treats as 0 — so the test still behaves correctly, just noisily).

Repro:

$ (actual_chars=0; meta_chars=0; pct=$(( actual_chars > 0 ? 100 / actual_chars : 0 )); echo "pct=$pct")
bash: actual_chars > 0 ? 100 / actual_chars : 0 : division by 0 (error token is ": 0 ")
pct=

In practice this can't fire — the marketing HTML pages are never 0 bytes, and the [ -f "$f" ] || continue guard covers nonexistent files. But a future page added to the FILES_TO_SCAN list that happens to be empty would emit confusing stderr noise during CI. Cleaner shape:

if [ "$actual_chars" -eq 0 ]; then
  echo "  SKIP: $f — 0 bytes, skipping drift check"
  continue
fi
diff_chars=$(( actual_chars > meta_chars ? actual_chars - meta_chars : meta_chars - actual_chars ))
pct_chars=$(( diff_chars * 100 / actual_chars ))

Non-blocking — file the follow-up only if a 0-byte page is ever realistic.

2. Copy-for-AI button accessibility (site/{index,architecture,skills}.html). The button has type="button" + readable text label which covers screen-readers via accessible-name computation, but:

• No aria-label — the text label "Copy as Markdown for AI" is sufficient as accessible name (don't add aria-label redundantly; that would override the visible text). Current state is correct on this axis.
• No visible focus state — the inline styles set border: 1px solid currentColor but no :focus / :focus-visible outline. Keyboard users tab-navigating the page won't see where their focus lands. Browsers do show their default focus ring on <button> even with custom border, so this likely works in practice, but an explicit :focus-visible rule would harden it.

Non-blocking but worth a follow-up.

3. UX during slow fetch (site/copy-for-ai.js:33). The button is disabled only AFTER the fetch completes and the flash starts — there's no spinner or "Fetching..." label during the network call. For typical .md alternates (< 100KB), this is invisible on broadband; on slow connections the user might click twice before the first fetch returns. The second click fires another fetch. Minor — flash duration (1.5s) bounds the misbehaviour, and Promise coalescing would be overkill for a marketing-site button. Non-blocking.

4. agent-permissions.json schema is the right shape for a still-firming convention. I reviewed the spec landscape: the GEO/AEO conventions around agent-permissions.json are not yet RFC'd; multiple proposals exist (the most-cited one — discussed in the GEO-audit registry — uses exactly the {schema_version, agents.{<glob>:{allow, rate_limit, preferred_endpoints}}} shape this PR ships). "*" as the catch-all agent is universally accepted. null for rate_limit correctly signals "unlimited" vs an integer per-minute cap. The 5 preferred_endpoints are absolute URLs (good — agents that resolve relative paths inconsistently won't trip). No schema concerns.

5. Item A skip rationale is sound. I considered whether any minimal framing of ai-plugin.json would help apexyard. The OpenAI plugin spec is fundamentally about hosted services with an OpenAPI surface — apexyard is markdown + shell distributed via git clone. The cleanest framings ("apexyard as a plugin that helps users set up SDLC tooling") would require a hosted backend the framework doesn't have. The skip + agent-permissions.json covers the discoverability story without misleading agents about apexyard's deployment model.

Verdict

APPROVED at HEAD ec31efa98f9b46c25b4daf9264341725a3774f3f.

(Submitting as a comment because GitHub blocks Rex from formally approving own PRs — operator will write the approval marker on Rex's behalf per the standard sandbox workaround.)

Scope discipline is excellent — no drive-by edits, all changes are AC-scoped, the skip decision is documented in the PR body (not relegated to comments). CI green at HEAD. Smoke test verified to fail correctly on both the missing-meta and drift-exceeded paths. Per-page meta accuracy is ~0.6% drift, well inside the 5% tolerance window.

The suggestions above (latent zero-divide noise, focus-visible style, slow-fetch UX) are all non-blocking polish items. None should hold this PR.

Reviewed by Rex (Code Reviewer Agent)
Reviewed commit: ec31efa98f9b46c25b4daf9264341725a3774f3f