fix(cloud): dashboard cleanup, CI fix, and Google OAuth sign-in - #412
Merged
Conversation
…stic compile
Priority 3. Ghost could execute and approve, but nobody could record — the
only way to get a workflow into Ghost was to upload a trace file produced by
some other tool. There was no capture at all, and `RecordingStatus.ACTIVE` was
never set because nothing recorded.
Per the capture decision in docs/ARCHITECTURE_DECISIONS.md §3, this ships a
Chrome extension for v1, execution stays entirely server-side.
## The trace contract (`@ghost/core/recording/trace`)
A Zod schema for what a recorder uploads: navigate/click/input/select/submit
events, each carrying a `TraceTarget` with accessible role, name, test id and
ordered CSS fallbacks. The defining property: **role and name are read at
capture time**, off the live DOM while the element is still on screen — not
inferred later from an opaque trace. Those are exactly the fields
`resolveLocator` already prefers, so nothing downstream had to change.
A secret is never captured, not encrypted or truncated — absent. `redacted:
true` records that something was typed without recording what. This is the
only place that redaction can be done honestly: a trace carrying the value
plus a "please ignore this" flag has already leaked it (see the P0-2 finding
in the architecture doc, about the previous design uploading traces whole to
a third party).
## The compiler (`@ghost/core/recording/compile`)
Trace to typed steps, deterministically. No model, no network, no configured
compiler. Three things it does beyond translation:
- collapses a run of keystrokes on one field into one `fill` with the final
value
- runs every produced step through `classifyStep` — the same deterministic
classifier the worker consults at run time — and inserts an `approval`
immediately before anything it would gate, so the authored workflow
agrees with what execution will actually do
- never carries a redacted value; a secret field becomes a `fill` marked
`sensitive` with a placeholder, and a note tells the reviewer to set it
This is what makes recording work with no compiler configured — the state
production runs in per the HarnessRouter decision (§1). A model still has a
job: naming steps, proposing gates beyond the obvious cases, flagging what it
could not resolve. Deciding which element was clicked is not a judgement call
and should never have been one.
24 tests, including a fixture shaped exactly like the extension's actual
output (`roundtrip.test.ts`) — the seam most likely to rot, since the
extension is untyped JS with nothing else to notice if its output drifts from
what the compiler expects.
## Ingest (`lib/recording-ingest.ts`)
Shared by the existing upload form and the extension, so upload limits,
filename sanitisation and audit events cannot drift between the two paths. A
structured Ghost trace compiles inline and lands `READY` in the same request;
anything else (HAR, Playwright zip) is stored and left for whatever compiler
is configured, unchanged from before.
`POST /api/agent/recordings` is new: the extension's ingest point, on the
already bearer-authenticated agent surface rather than the session-only
upload route, since the extension runs at a different origin and cannot rely
on the session cookie reaching it. `resolveAgentPrincipal` enforces the same
second factor on its session fallback as every other agent route (see the
P0-1 fix). Uploading only creates a proposal — nothing here publishes a
workflow or executes anything; the human still reviews compiled steps in the
editor and publishes through `POST /api/workflows`, which revalidates them.
## The extension (`apps/extension`)
Manifest V3. `content.js` captures clicks, typing, selects, submits and SPA
navigation, computing accessible name in roughly accname-spec order from
label/aria-label/aria-labelledby/placeholder/value/text. Secret fields are
detected by `type=password`, secret-shaped `autocomplete`, or a name/label/id
matching a word list (password, otp, cvv, card number, ssn, ...) — and the
value is never read for them, not merely withheld after reading.
`background.js` buffers events locally and uploads only on Stop, via a
revocable bearer token created in Ghost Settings. The popup is deliberately
thin — start, stop, where to send it — because a second place to edit steps
would be a second thing to keep in sync with the real editor.
## Validation
398 tests pass (up from 374 — 24 new), against a database migrated from
zero. `pnpm typecheck` and `pnpm build` both clean with `HR_API_KEY` unset.
Manually verified `ingestTrace` end-to-end against a real Postgres: a
structured trace lands `compileStatus: READY` with steps persisted, in one
request, no compiler configured.
## What is not yet covered
`GET /api/recordings` gained a `take: 50` (P2-2 for this route) as an
incidental fix while touching the file; the rest of that finding stands.
Extension host permissions are `<all_urls>` for v1; scoping to allowlisted
origins per organization is a natural follow-up once there is a customer to
scope it for. No component tests for the extension itself — Manifest V3
content scripts have no test harness in this repo, so `roundtrip.test.ts`
pinning its exact output shape is the coverage that exists.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The worker Dockerfile had never actually been built or run since it was written. Auditing what was genuinely left on the cloud roadmap surfaced two stacked bugs, neither caught by CI (`pnpm build` only bundles the worker with tsup at the workspace level — it never exercises the Dockerfile's own COPY list or executes the resulting dist/index.js): - The deps stage copied packages/core/package.json but not its prisma/ directory. @ghost/core's postinstall runs a bare `prisma generate`, which resolves the schema at the default ./prisma/schema.prisma path, so `pnpm install` failed inside the image before the build stage (which does copy the rest of packages/core) ever ran. The same gap silently dropped tsconfig.base.json, breaking apps/worker/tsconfig.json's `extends`. - Once building, the container crashed immediately on boot: bundling @ghost/core (via tsup's `noExternal: [/^@ghost\//]`) pulls in @prisma/client's generated CJS runtime, which dynamically requires native query-engine files — esbuild's CJS-to-ESM interop can't represent that and throws "Dynamic require of 'fs' is not supported" at the first call. Fixed by keeping @prisma/client external in tsup.config.ts and adding it as a direct dependency of @ghost/worker, since pnpm's strict linking won't resolve a transitive dep at the worker's own require path otherwise. Verified by rebuilding the image and running it against real Postgres/Redis until it logged its startup line rather than crashing. Added a CI step that builds and boot-smoke-tests the image on every PR so this class of bug can't ship invisibly again. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…implementation CURSOR_HANDOFF.md described the WorkflowCompiler abstraction, four-eyes approval, and S3 artifact serving as open work, framing a stale roadmap that led to prioritizing already-shipped items. All three were fully implemented and tested by earlier PRs (#403, #406) whose changes never made it back into this file's "remaining work" section: - WorkflowCompiler: apps/web/src/lib/compiler/{types,index,harness-router-compiler}.ts already provides the interface, a swappable HarnessRouter adapter, and normalized error types. - Four-eyes approval: Membership/Role/Invitation plus Organization.requireSeparateApprover already enforce requester != approver server-side, covered by separation-of-duties.test.ts. - S3 artifact serving: packages/core/src/storage/artifacts.ts already has a working S3ArtifactStore with presigned URLs; only retention/cleanup is genuinely still open, called out as such. Also documents the worker container bugs found and fixed in the prior commit, and corrects the stale "239 tests" figure (actual full green run is 398) and the "typed step editor: remaining" status line (workflow-editor.tsx already covers it) in both this file and README.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
In production with no GitHub OAuth app configured, the signin page rendered a card with a title and zero buttons -- indistinguishable from a bug. Whoever hits this is more likely to be standing up the deployment than an end user, so name the exact fix (AUTH_GITHUB_ID/AUTH_GITHUB_SECRET, the callback URL) instead of failing silently. See docs/DEPLOY.md's "sign-in trap". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…Sentry error tracking Three gaps identified by an audit of what was genuinely left on the cloud roadmap (recorded in CURSOR_HANDOFF.md), closed as one changeset since they share the same few files (index.ts, purgeArtifacts.ts): - Retention: nothing ever deleted a run's screenshots. `purge-artifacts` is a new BullMQ job, scheduled daily via upsertJobScheduler at worker boot, that deletes a run's artifact prefix once it ended more than ARTIFACT_RETENTION_DAYS ago (default 90) and audits the deletion. Run. artifactsPurgedAt makes it idempotent; a store failure is retried next cycle rather than silently marked done. Eligibility is a pure function (packages/core/src/retention.ts) so the window logic is tested without a database. - Structured logging: the worker only had console.log/console.error, so a failure was invisible unless someone was tailing container logs. packages/core/src/logger.ts is a small, dependency-free JSON-line logger (errors to stderr, everything else to stdout) now used throughout index.ts and purgeArtifacts.ts. - Error tracking: @ghost/core/sentry wraps @sentry/node, gated on SENTRY_DSN exactly like HR_API_KEY/S3_BUCKET -- absent means a complete no-op, present enables capture on every job-failure handler. Worker-only: wiring apps/web needs @sentry/nextjs (its own webpack/turbopack plugin exists specifically to handle Sentry's auto-instrumentation, which cannot otherwise be bundled -- confirmed by trying the manual approach first and watching it break `pnpm build` with an unbundleable node:child_process import). @sentry/node gets the same tsup `external` treatment as @prisma/client, for the same reason: its runtime does dynamic requires that bundling breaks. Verified end-to-end: full test suite (424 tests) against real Postgres + Redis, a Docker rebuild, and a boot smoke test with SENTRY_DSN actually set (not just absent) to confirm Sentry initializing doesn't crash the container. DEPLOY.md, README.md and CURSOR_HANDOFF.md updated for the new env vars and the corrected test count. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ing-extension # Conflicts: # cloud/README.md # cloud/apps/worker/package.json # cloud/apps/worker/tsup.config.ts # cloud/docs/CURSOR_HANDOFF.md # cloud/packages/core/package.json # cloud/pnpm-lock.yaml
Discovered by performing the repo's first real deployment (Vercel project
ghost-app, Neon Postgres, Upstash Redis) rather than just writing a checklist
for one:
- cloud/apps/web needs its own vercel.json ({"framework": "nextjs"}).
DEPLOY.md previously claimed this wasn't necessary based on reading Vercel's
monorepo docs, but empirically: with the repo root's own vercel.json present
(the marketing site's static-build config) and Root Directory set but no
local vercel.json, a build falls back to the root's install/build commands
against the wrong project's uploaded files -- Vercel prints "The vercel.json
file should be inside of the provided root directory" as a warning, then
uses it anyway. A local vercel.json closes the gap outright.
- Added .vercelignore at the repo root. `vercel deploy` uploads the working
directory as-is, not `git ls-files`, so untracked local dev artifacts
(.worktrees/, .wt/, .turbo/ -- each a separate git-worktree checkout with
its own node_modules/target) get swept into the upload. Without this, a
deploy from the repo root uploaded 57,896 files instead of ~700.
- Vercel's own Deployment Protection (SSO/Vercel Authentication) was on by
default for the new project, gating every page behind a Vercel-account
login wall on top of Ghost's own auth -- disabled for ghost-app.
DEPLOY.md updated with all three findings, plus removing the stale "no
deployment exists yet" framing now that apps/web is live (worker is not; no
container host is wired up, and object storage was deliberately deferred).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ing-extension # Conflicts: # cloud/docs/DEPLOY.md
The version-consistency check hard-failed when public/index.html didn't contain a vX.Y.Z string, cross-checked against README.md. That assumption broke on master after the site copy was rewritten to drop legacy desktop positioning entirely (following the download-CTA removal in #396) -- the site no longer mentions a specific release at all, which is the intended current state, not drift. README.md still names the published desktop tag as historical reference; nothing there needs to change. Only cross-check the two when the site actually advertises a version, so a real future mismatch still fails loud. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The dashboard showed "Phase 1"/"Phase 2" badges next to each capability and a raw "Enqueue test job" wiring-check button -- internal build-tracking language and an engineering diagnostic, both visible to any signed-in user, including a real customer or design partner. Neither belongs on a product surface. Kept the underlying /api/dev/enqueue-noop route (still authenticated, still useful for verifying the web-Redis-worker path via curl after a deploy) -- only removed what put it on the front page. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
GitHub-only sign-in is a barrier for anyone without a GitHub account. Google is registered independently of GitHub, following the same env-var-gated pattern already used for GitHub — a deployment can offer either, both, or neither, and the sign-in page's misconfiguration warning now names both. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…ing-extension # Conflicts: # cloud/docs/DEPLOY.md
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.
Summary
fix(web): stop leaking internal roadmap status onto the dashboard)Test plan
pnpm --filter @ghost/web exec tsc --noEmit— cleanpnpm --filter @ghost/web exec eslint src/auth.ts src/app/signin/page.tsx— cleanpnpm typecheck(all packages) — cleanpnpm test(all packages, Postgres + Redis running) — 5/5 packages greenpnpm --filter @ghost/web build— succeeds🤖 Generated with Claude Code