Skip to content

Resolve the deferred v1 review fixes - #16

Merged
dorelljames merged 3 commits into
mainfrom
symphony/gh-9
Oct 30, 2026
Merged

dorelljames merged 3 commits into
mainfrom
symphony/gh-9

Conversation

@dorelljames

@dorelljames dorelljames commented Oct 30, 2026 •

Copy link
Copy Markdown
Member

Closes #9

What: Fix canonical fallback/template URLs, preserve legacy slash URLs, and close the v1 type-check, build-check, CI-runtime and deployment-review gaps.
Checked: Frozen install, 170 tests (including Astro/TypeScript), build and all 36 local Workers URL assertions pass; Astro dev/preview checked separately; whole-branch and whitespace review complete.
Look at: The approved brand 0.1.1 release and template behavior, plus the www-only configuration guard and @dorelljames ownership entries.

Details
  • apps/website/worker/index.ts requests canonical /404; its direct handler test verifies origin, status, body and headers independently of asset-first misses.
  • packages/brand/src/brand.ts, package metadata and generation tests publish extensionless event-post/fb-cover URLs as release 0.1.1 with a changelog. Physical HTML assets stay in place. The brand page recognizes templates by path prefix so their links open without download attributes; check-dist exercises the rendered links.
  • _redirects and the pinned test now contain all twenty source forms, preserving destinations and 301/302 statuses. _headers gives tokens CSS its charset and retains wildcard CORS. url-matrix.sh covers these and canonical templates without Location headers.
  • Website package scripts run Astro sync, Astro check and tsc before Vitest. @astrojs/check@0.9.12 supports TypeScript 5/6, so website scopes TypeScript 6.0.3; root keeps 7.0.2. Generic image schema factories and collection callbacks preserve production image metadata and string test stubs without suppressions. The lockfile includes the checker and JSONC tooling dependencies.
  • check-dist.ts resolves extensionless HTML separately from physical file requirements, validates redirect fragments (including encoded/malformed IDs), checks same-site canonical sitemap XML entries, and rejects downloadable template links. Brand lint and the website checker share the existing color detector through findColorLiteral; scan contexts and line diagnostics remain covered.
  • Lead answers 1A/2A are recorded in the plan/spec and workpad: keep Facebook, X, GitHub and Email; correct the original v1 spec; pin the exact four content URLs. CODEOWNERS assigns Wrangler and CODEOWNERS review to @dorelljames. The structural JSONC guard detects changed/extra domains, top-level singular route and nested route/routes overrides. wrangler.jsonc, DNS and branch-protection settings are unchanged.
  • CI uses checkout v7.0.1, setup-node v7.1.0 and pnpm/action-setup v6.1.0. Their official action.yml manifests all declare node24; release/migration notes were reviewed. Project Node 24, pnpm 11.3.0, frozen install, tests and build remain configured. Hosted CI on this implementation is not yet observed.
  • Claude round 1's must-fix is implemented and covered by fixture tests plus the built-page check. Its dev/preview and singular-route suggestions are covered. Round 2's resolver ordering, explicit patch-release approval and conditional spec wording are incorporated; the add-Website suggestion is inapplicable to 1A.

Validation: pnpm install --frozen-lockfile passes; pnpm test passes 112 brand + 58 website tests in 15 files. Astro checks 39 files with 0 errors, 0 warnings and 3 existing URL API deprecation hints; tsc passes. pnpm build passes check-dist (15 required files, 6 pages, 20 redirects and 30 source files). All 36 Workers local URL assertions pass, including both templates 200 without redirects, CSS charset/CORS and genuine missing-page 404.

Astro dev: /brand/ returns 200 but both extensionless templates, including the cover iframe URL, return 404 without redirects. Astro preview: all three return 200 without redirects. Both rendered pages have canonical template links without download and the canonical cover iframe/link. Workers serves the templates correctly; Astro dev's static-file mapping limitation is recorded rather than changing the canonical public URLs.

Environment cleanup blocker: Workers stopped. Astro auto-detached its local processes; sandbox EPERM prevents terminating PIDs 29334 (dev, localhost:4391) and 29496 (preview, localhost:4392), and Astro's stop command cannot detect them through its denied liveness probe. Their HTTP checks are complete. No implementation/access/secret blocker remains.

The full diff against origin/main and all untracked source files were reviewed; no generated output, secrets or unrelated changes are included. The host will commit/push this branch and update this existing PR after the run; no deployment or merge was performed.

An agent prepared this PR through Symphony. It needs a lead's review before merging.

Describe all nine GH-9 fixes, regression coverage and validation without changing implementation before plan approval.

Record the official-channel and configuration-owner decisions for a lead, and supersede the earlier missing-skills blocker.
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-30T09:46:51.936766Z 80f48a3 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

🚀 Deploying Preview to Cloudflare 🚀

Preview URL: https://symphony-gh-9-jscebu-org.dorelljames.workers.dev (commit aab6183)

This URL reflects your latest Preview deployment

Preview Deployments by commit

Status Deployment URL Commit Updated (UTC) See this deployment's details
  • Build: Success ✅
  • Deployment: Success ✅

View logs ↗
https://452c2a0d-jscebu-org.dorelljames.workers.dev aab6183 2026-10-30T14:58:23.663Z Visit the dashboard ↗
  • Build: Success ✅
  • Deployment: Success ✅

View logs ↗
https://edcfc12a-jscebu-org.dorelljames.workers.dev 4ad1ef2 2026-10-30T09:53:14.124Z Visit the dashboard ↗
  • Build: Success ✅
  • Deployment: Success ✅

View logs ↗
https://5e1db1dc-jscebu-org.dorelljames.workers.dev 80f48a3 2026-10-30T09:46:07.866Z Visit the dashboard ↗

@dorelljames

Copy link
Copy Markdown
Member Author

Claude review (round 1)

Summary

The plan covers all nine issue items, stays within docs/superpowers/, and lists the two lead-only decisions, but following Task 2 as written would regress the template links on /brand/. I couldn't confirm the plan's action versions or @astrojs/check compatibility with TypeScript 7, because gh isn't authenticated and the npm registry is blocked here; the plan does require both to be rechecked during the build.

Findings

  • [must-fix] docs/superpowers/plans/2026-10-30-v1-review-fixes.md:65: Task 2 changes the two template paths to extensionless, but apps/website/src/pages/brand/index.astro:194 decides whether to download with a.path.endsWith('.html'). Both template links under "Templates & downloads" would then get download="", so they download instead of opening. That page isn't in the file map (line 38), and no test or acceptance item covers download, so the build would pass with the regression. Add the file to the plan, key the condition on something that survives the path change (such as the /brand/templates/ prefix), and assert that template links have no download attribute.
  • [suggestion] docs/superpowers/plans/2026-10-30-v1-review-fixes.md:65: The /brand/ iframe (apps/website/src/pages/brand/index.astro:162) and template links will use the extensionless URLs. pnpm --filter website dev and astro preview probably don't apply the Workers asset rule that maps those URLs to .html, so the local preview may 404 (not run). Have the build check this and record the result, so it isn't mistaken for a regression.
  • [suggestion] docs/superpowers/plans/2026-10-30-v1-review-fixes.md:107: The guard checks for an environment or preview route array, but Wrangler also accepts a singular route key, which an array-only check would miss. Reject route as well as routes under env and previews, and add it to the failure cases on line 108.

VERDICT: changes-needed

Sent back to Rework (send-back 1 of 2).

@dorelljames dorelljames mentioned this pull request Oct 30, 2026
9 tasks
Plan stable template link behavior with built-output regression checks, separate Astro preview verification, and singular and plural route override guards.
@dorelljames

Copy link
Copy Markdown
Member Author

Claude review (round 2)

Summary

The plan and spec cover all nine issue items, change only docs/superpowers/, and include the round 1 download fix; both real lead decisions (Q1, Q2) are listed at the top of the plan. I checked its claims against the code: the five tsc errors reproduce, the color regexes are identical, and Wrangler's parser treats slash rows as distinct exact matches; I did not rerun pnpm test or pnpm build, since nothing outside the docs changed.

Findings

  • [suggestion] docs/superpowers/plans/2026-10-30-v1-review-fixes.md:68: This step says to rerun the build, but the resolver only arrives in the next two steps. Until then exists() in check-dist.ts reports /brand/templates/event-post as not built, so the build fails here and the failure on line 67 is mixed with link errors. Moving the resolver steps ahead of the metadata change would keep each failing test about one cause.
  • [suggestion] docs/superpowers/plans/2026-10-30-v1-review-fixes.md:31: The 0.1.1 patch bump is only proposed here, not listed under the open questions. CONTRIBUTING.md gives v0.1 → v0.2 as its release example, and this release also adds a public export. Naming the version in the approval block would stop a lead from approving it without noticing.
  • [suggestion] docs/superpowers/plans/2026-10-30-v1-review-fixes.md:102: The add-Website branch doesn't mention the v1 spec's / row, which lists Join as "Facebook, GitHub, X, email". Adding Website to the shared collection would make that row wrong, and only the general line 104 catches it. Name that row in this branch.
  • [suggestion] docs/superpowers/specs/2026-10-30-v1-review-fixes-design.md:10: "Explicitly approved clarification to its /links row" describes an approval that hasn't happened, because Q1 is still pending. The clarification also won't exist if Q1 adds Website, so this should read as conditional on Q1.

VERDICT: ready-for-human

Ready for a lead.

Publish canonical template URLs with brand 0.1.1, preserve slash redirects, and validate redirect fragments, sitemap entries and template navigation.

Include Astro and TypeScript checks in tests, share color detection, modernize CI action runtimes, and guard the approved www-only deployment surface with tests and CODEOWNERS.

Record approved decisions, regression and local HTTP evidence, the Astro dev template limitation, and sandbox-blocked detached server cleanup.
@dorelljames dorelljames changed the title Plan: resolve deferred v1 review fixes Resolve the deferred v1 review fixes Oct 30, 2026
@dorelljames

Copy link
Copy Markdown
Member Author

Claude review (round 3): ready for a lead

All nine issue items are done and I found nothing that should block merge, but hosted CI hasn't run on this commit, so a lead should see it pass first.

No must-fix findings.

Why, and suggestions
  • [suggestion] apps/website/src/pages/brand/index.astro:162: Under pnpm --filter website dev, the cover preview and both template links return 404, by the author's own check. Production is unaffected and the PR discloses it, but AGENTS.md calls dev the local preview, so a one-line note there would save someone a false bug hunt.
  • [suggestion] apps/website/scripts/check-dist.ts:56: resolveBuiltPath accepts /about/index and /index (confirmed by running it), though its comment promises no redirect hops. Nothing links to them today; rejecting a final index segment would close the gap.
  • [suggestion] apps/website/scripts/check-dist.ts:5: The header comment still lists the old checks. Add redirect fragments, sitemap entries and template links.

What I checked

  • Clean-state run: I used a temp copy of the branch with dist, .astro and public/brand removed, running each step in the order pnpm test and pnpm build do. astro sync and astro check passed with 0 errors and 3 hints, and tsc passed. All 170 tests passed (112 brand, 58 website), as did brand sync, astro build and check-dist.
  • New checks can fail: check-dist failed when I added a missing redirect fragment, a slash-less sitemap URL, a .html link in brand.md, a deleted template and a download attribute on the template link. The tests cover the route guard's failure cases, and I read them rather than mutating the guard.
  • Issue items by reading the diff:
    • All 20 redirect rows keep their destinations and statuses.
    • The _headers rule is correct.
    • No stale .html template URLs or old version strings remain.
    • wrangler.jsonc is untouched.
    • The URL matrix makes the 36 assertions the PR claims.
  • Brand release: the changelog entry and versions match. The 0.1.1 approval is the author's inference from the lead approving the plan, and the PR's "Look at" line flags it.
  • Lockfile: it adds the checker tooling and typescript@6.0.3, and no existing package version changes.

What I could not check

  • Hosted CI: origin/symphony/gh-9 is still at 4ad1ef2, so aab6183 hasn't run there.
  • GitHub claims: GitHub is blocked here and gh isn't logged in. I could not confirm the action tags or their node24 manifests, or read the lead's 1A 2A comment. A wrong tag would show up as a CI failure.
  • Frozen install: pnpm install --frozen-lockfile could not reach the registry. The importer specifiers for @astrojs/check, jsonc-parser and typescript do match package.json.
  • Live HTTP behaviour: Wrangler cannot bind a local port here. Redirects, the CSS charset, extensionless templates and the 404 fallback through Workers rest on the author's reported 36 passing assertions; their saved log shows no requests I could match.

VERDICT: ready-for-human

@dorelljames
dorelljames merged commit 09cd7ee into main Oct 30, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Small fixes from the v1 review

1 participant