feat(GLA-1098): asset-library auto-rebuild watcher + chunk-error UI + CI smoke - #5535
feat(GLA-1098): asset-library auto-rebuild watcher + chunk-error UI + CI smoke#5535jqueguiner wants to merge 28 commits into
Conversation
The partial unique index `issues_open_routine_execution_uq` previously
required execution_run_id IS NOT NULL, leaving a window where two
routine_execution siblings sharing (company_id, origin_id,
origin_fingerprint) could both be inserted with null execution_run_id. A
later UPDATE that populated execution_run_id (and kept the row in the
index) admitted both rows, wedging every subsequent write — including
the reaper's PATCH {status:"cancelled"} — with 23505.
- Migration 0082 hides newer duplicates and recreates the partial index
without the execution_run_id IS NOT NULL clause so collisions fire at
INSERT time. Rollback is documented inline.
- Schema mirrors the new predicate.
- Dispatcher coalesce path adds a fallback finder so a partial-unique
conflict before execution_run_id is populated still resolves to the
pre-existing sibling instead of bubbling 23505.
- Error handler lifts postgres-js error fields (code, constraint, table,
detail) so future failures surface in app frames instead of bare
driver stacks.
- Issue update route logs origin tuple + pg fields when svc.update
throws an unexpected error, surfacing the failing call site.
- Regression test covers (a) duplicate sibling INSERT now fails with
23505/issues_open_routine_execution_uq, (b) cancellation PATCH on a
sibling whose execution_run_id is populated returns successfully, and
(c) hidden prior siblings free the index slot.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…GLA-441) Add scoped bypass in assertAgentIssueMutationAllowed: when the caller is a security-role agent and the target issue is critical or high priority, allow POST /comments through with an audit log row (cross_assignee_security_comment, safeguard_role_bypass=true). Gate stays scoped to comments — PATCH issue, PUT documents, and other mutation paths still 403. Co-Authored-By: Paperclip <noreply@paperclip.ing>
Tighten GLA-441's role=security cross-assignee comment bypass: now also requires the caller's agent id be in SAFEGUARD_COMMENT_ALLOWLIST (only the SafeguardReviewer for now). Other security-role agents (e.g. SecurityEngineer) lose the bypass and get the standard 409 path. Stamp comment metadata.safeguardBypass=true on the bypassed insert so reviewers can see at a glance why a non-assignee posted; widens the audit log row from GLA-441 with a per-comment artifact. Schema gets a new optional safeguardBypass boolean on IssueCommentMetadata. Tests: 8 cases — allow on high+critical with metadata stamped, deny on medium, deny non-allowlisted role=security, deny cross-company, deny PATCH, deny engineer, plus assignee-self path unchanged (no bypass flag). Co-Authored-By: Paperclip <noreply@paperclip.ing>
Productivity reaper now skips long_active_duration triggers when the source issue has at least one non-terminal blocker via issue_relations, or at least one non-terminal direct child assigned to a different agent. no_comment_streak and high_churn triggers are never downgraded. Emits a structured productivity_review.suppressed_long_active log line with reason, blockerCount, and nonTerminalChildCount when the suppressor fires. Implements Option A from GLA-597. No schema change. Rollback: git revert. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ssor tests - Export listUnresolvedBlockerIssueIds from server/src/services/issues.ts so productivity-review can reuse the cancelled-blocker-stays-unresolved semantic the rest of the issue service already relies on. - Import the helper in productivity-review.ts (call site at collectEvidence was missing the symbol after GLA-599 landed, so the suppressor path threw ReferenceError on the blocker branch). - Remove three duplicate long_active suppressor tests now covered by the GLA-599 test set; keep the GLA-437 multi-child + blockedBy repro test. Verification: pnpm --filter @paperclipai/server exec vitest run productivity-review-service (17/17 green). Rollback: git revert.
PAPERCLIP_API_KEY was captured as '' at module init (line 19) before the env-sourcing block ran (line 34+). Moved sourcing into an IIFE-style block immediately after imports so all process.env keys are populated before any const reads them. Also added PAPERCLIP_API_KEY to notifier.env so launchd/cron invocations (which don't inherit the interactive shell env) pick it up automatically. Fixes slot-1650 failure reported on GLA-77. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add plugin-github-sync: manifest with events.subscribe, plugin.state.read/write, secrets.read-ref, http.outbound, goals.read capabilities; instanceConfigSchema for repo/host/secretRef/syncedGoalIds/dryRun; no-op handlers for issue.created, issue.updated, goal.updated that log sanitised payload and resolve goal-ancestor chain. 4/4 unit tests pass. Co-Authored-By: Paperclip <noreply@paperclip.ing>
…ated test - Add tests/github-client.spec.ts: 19 tests covering PAT redaction, rate-limit parsing, backoff on 429/403, createIssue/closeIssue/ reopenIssue/addLabel methods, redirect:error enforcement. - Fix existing plugin.spec.ts goal.updated assertion: the handler logs "github-sync: goal.updated — cache invalidated", not the generic no-op message. Test now checks the correct message for each event type. All 23 tests pass. Co-Authored-By: Paperclip <noreply@paperclip.ing>
…ADME depth docs - Add goal-subtree-cache.spec.ts: 12 tests covering 3-level tree (in-scope branch syncs, out-of-scope does not, root-less skipped), cache hit/miss, TTL expiry, invalidation scoped per company, 5-level depth fixture, and cycle-guard termination - Document real-world goal tree depth (2–3 typical, 5 max) in README Co-Authored-By: Paperclip <noreply@paperclip.ing>
… sanitisation - Add src/github-client.ts (missed from prior commits) - Add src/goal-subtree-cache.ts (missed from prior commits) - Add src/sync-engine.ts: sanitiseBody, buildTitle, mapIssueStatus, createSyncEngine with doSync + scheduleSync (debounce 2 s per issue) - Update src/manifest.ts: add issues.read capability - Refactor src/worker.ts: replace no-op handler with engine.scheduleSync via routeEvent; keep company/goal-subtree filter chain intact - Add tests/sync-engine.spec.ts: 23 tests covering sanitiser unit tests, idempotency (create + 3 replays = 1 POST), audit log, status transitions (done/cancelled/open), dry-run gate, debounce - Update tests/plugin.spec.ts: replace no-op assertions with new dry-run log assertions, out-of-scope short-circuit, goal.updated cache 60/60 tests pass; typecheck clean. Co-Authored-By: Paperclip <noreply@paperclip.ing>
…tart
W1A of GLA-927 — unblock the marketing freeze (GLA-964) by exposing a
local UI for the Paperclip [review-and-ship] queue.
- scripts/asset-library: Next 14.2.18 + React 18.3.1 + Tailwind 3 app router
- /: placeholder ("Asset Library — coming soon")
- /api/issues: server-side proxy to PAPERCLIP_API_URL companies issues,
filtered to titles starting with [review-and-ship]
- ecosystem.config.js: pm2 wrapper, pinned to /opt/homebrew/opt/node@20,
loads notifier.env for PAPERCLIP_API_KEY etc.
- launchd/io.gladia.asset-library.plist: autostart on login via
`pm2 resurrect || pm2 start`
- README documents run/install/verify
Verified: curl http://127.0.0.1:7700/ returns 200; /api/issues returns
the [review-and-ship] subset.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
…ration - POST /api/companies/:id/integrations/github — set/replace config (repo, host, secretRef, syncedGoalIds[], dryRun). Validates repo shape, secretRef resolves to a secret in the company, syncedGoalIds exist in the company. - GET /api/companies/:id/integrations/github — read config. secretRef returned (UUID ref only), PAT value never surfaced. - DELETE /api/companies/:id/integrations/github — disable integration; existing GH issues stay in place. - POST /api/issues/:id/sync-to-github — synchronous reconcile for one issue. dryRun:true returns planned payload with no GH API call. Live runs resolve the secret, upsert the GH issue, and update plugin state + lastError. Config stored in pluginCompanySettings.settingsJson for paperclipai.plugin-github-sync. Board-only auth, company-access checked on every route. logActivity calls record configure/disable events. UI: adds "GitHub sync (one-way, beta)" section to CompanySettings with repo, host, write-only PAT (existing secret picker or new PAT entry), goal multi- select, dryRun toggle, status row (last error + last sync timestamp), and disable button. API client added at ui/src/api/github-integration.ts. Co-Authored-By: Paperclip <noreply@paperclip.ing>
Founder hit ENOENT loop after `pm2 resurrect` because a fresh checkout has no `.next/` and `next start` cannot run without it. Manual `npm run build` unblocked it. This wraps the start path so pm2/launchd handle the build themselves. - start.sh: pin node@20, run `npm run build` if `.next/BUILD_ID` is missing or any source under `app/` / config files are newer than the build, otherwise exec `next start`. Soft-fail: if rebuild fails but a previous `.next/` exists, log + serve the stale artifact instead of pm2-looping; refresh BUILD_ID mtime so we don't retry on every restart. - ecosystem.config.js: route pm2 through start.sh (`interpreter: bash`), drop the explicit node interpreter — start.sh sets PATH itself. Verified: pm2 restart picks up start.sh, logs "build is fresh — skipping rebuild" when artifact is current; with the current broken `lib/forbidden-source-scan.ts` TS error in W1B work, start.sh logs the fallback warning and still serves :7700 → 200. Re GLA-981 post-mortem. Co-Authored-By: Paperclip <noreply@paperclip.ing>
Adds upload-time scan rejecting Shutterstock / Unsplash / Getty / Adobe Stock assets before they hit Paperclip storage. Filename regex + EXIF URL inspection via exifr. New /api/upload route forwards clean files to the Paperclip attachments endpoint; matches return HTTP 400 with forbidden-source body. Acceptance smoke (scripts/scan-smoke.ts, scripts/upload-route-smoke.ts): - shutterstock_123456.jpg -> 400 forbidden-source - AdobeStock_98765.png -> 400 forbidden-source - GettyImages-99999_final.jpg -> 400 forbidden-source - clean local-flux-output.png -> not forbidden Co-Authored-By: Paperclip <noreply@paperclip.ing>
…brary board-report and email-notifier now detect [review-and-ship] / [marketing-asset] issues and emit asset-library detail-page links (http://127.0.0.1:7700/asset/{issueId}/{docKey}) instead of plain Paperclip issue links. - email-notifier: when a blocked issue is asset-review, build an [asset-review]-prefixed email with one CTA per document, inline thumbnail (PNG/JPG via base64 data URI, capped at ASSET_LIBRARY_THUMBNAIL_MAX_BYTES=200KB), "▶ Watch video" link for video attachments, and a secondary issue-thread link. - board-report: per-row primary anchor swaps to the asset library URL for asset-review issues; deliverables list becomes per-doc CTA links; rows are tagged [asset-review]. - ASSET_LIBRARY_URL env (default http://127.0.0.1:7700) drives both. Co-Authored-By: Paperclip <noreply@paperclip.ing>
…pulates The GET /integrations/github endpoint reads lastSyncAt from the pluginLogs table, but POST /issues/:id/sync-to-github never inserted rows there — neither in dryRun mode nor on live create/update/error paths. Insert an info-level log row before returning in all three paths so that the status row in Company Settings shows the last sync timestamp. Co-Authored-By: Paperclip <noreply@paperclip.ing>
Current owner can now PATCH assigneeAgentId to transfer a routine to another agent in the same company. Non-owners remain blocked. Covers all four matrix cases from GLA-1043 spec. Co-Authored-By: Paperclip <noreply@paperclip.ing>
Co-Authored-By: Paperclip <noreply@paperclip.ing>
…rary
Wires the W2B approve/reject flow on the marketing asset library detail
page. Hard-blocks approval when provenance is missing, names a cloud-API
tool with no valid waiver, or matches a stock-library filename pattern.
Mirrors the gate server-side so the API rejects 422 even if the client
is bypassed.
- lib/asset-type.ts: parseProvenance reads metadata.provenance / YAML
frontmatter / "Provenance:" block; evaluateApprovalGate returns
{allowed,status,banner,bannerTone,...} per AC matrix (missing | cloud
| cloud-with-exception | exception-pending | exception-invalid |
stock | ok). CLOUD_TOOL_PATTERNS matches Runway/Pika/Sora/Heygen/
Synthesia/Midjourney/DALL-E/ElevenLabs/Suno/Udio (case-insensitive,
partial). STOCK_FILENAME_PATTERNS catches shutterstock_<n>/
GettyImages-<n>/AdobeStock_<n>.
- app/api/exception-check/route.ts: server-side waiver lookup —
exception is valid only when status==done AND title contains
[tool-exception].
- app/api/issues/[id]/approve/route.ts: re-evaluates gate against
upstream document, posts approval comment, PATCHes status:"todo".
- app/api/issues/[id]/reject/route.ts: requires note, posts rejection
comment, PATCHes status:"in_progress". 4000-char note cap.
- app/asset/[id]/[doc]/ApproveRejectBar.tsx: button row + reject
modal; APPROVE disabled when !gate.allowed; banner toned ok/warn/
block; uses /api proxy routes (server holds PAPERCLIP_API_KEY).
- scripts/gate-check.ts: AC harness — all 7 cases pass
(missing/cloud-no-waiver/cloud-with-valid-waiver/cloud-with-invalid-
waiver/local-ai-ok/stock-blocks-even-with-local-ai/founder-original).
Detail page integration (page.tsx) and ProvenancePanel/AssetRenderer/
MarkdownView are W2A (GLA-984) deliverables — left untracked here for
that owner to land. Live verified against pm2 asset-library on
http://127.0.0.1:7700/asset/GLA-917/post-body — gate banner "Cannot
approve — missing provenance metadata" renders, APPROVE button
disabled.
Acceptance criterion for GLA-1026 was: pm2/launchd restart returns HTTP 200 with zero manual `npm run build`. start.sh already enforces this (commit 3580c7f). What was missing was a CI/local-runnable smoke that exercises the same chain end-to-end without touching the running pm2 process. - scripts/build-start-smoke.sh: runs `next build` then `next start -p 17700` (separate port from prod :7700), polls 30s for HTTP 200, kills the spawned server on exit. Exits non-zero on timeout. - package.json: `npm run smoke` calls the helper. Verified locally: `npm run smoke` → "[smoke] HTTP 200 on :17700 — ok". Re GLA-1026. Co-Authored-By: Paperclip <noreply@paperclip.ing>
…issues A routine's owning agent (e.g. OpsReaper) may now PATCH `status=cancelled` on its own routine_execution issues even when the issue is checked out by another assignee (e.g. SafeguardReviewer leaking executions during a crash loop). Scope is narrow: - only when `originKind=routine_execution` - only when the request body sets `status=cancelled` and nothing else (besides an optional `comment`) - only when the actor agent equals the parent routine's `assigneeAgentId` - routine company must match issue company Every use writes a `cross_assignee_routine_owner_cancel` activity log row with routineId, prior assignee, and prior status, so the bypass leaves an audit trail. Unblocks the safeguard reaper (GLA-1045) and closes the API permission gap surfaced by GLA-1029. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…l leaked routine_execution issues Adds a permission-grant carve-out alongside the existing routine-owner carve-out (GLA-1048) so an agent holding the new `tasks:cancel_routine_execution` permission can PATCH status=cancelled on a routine_execution issue assigned to a different agent. Spec §11.a. - New permission key: `tasks:cancel_routine_execution`. - `assertAgentIssueMutationAllowed` now accepts a `routineExecutionCancelByPermission` option and bypasses the foreign-issue 403 only when the issue origin is `routine_execution`, the requested status is `cancelled`, and the actor agent has the permission grant. Other fields beyond status / comment are rejected with 403 by the PATCH handler, matching the routine-owner carve-out. - Logs `cross_assignee_routine_execution_cancel` activity with the caller, prior assignee, prior status, and origin metadata. - Migration 0083 grants the permission to OpsReaper (`16d0232f-1249-4db9-82d6-ab237c926e59`) at the Gladia company (`050de589-23d3-40bb-b227-efea13164d01`). Conditional INSERT on company + agent existence; idempotent via the unique index. Rollback in the migration footer (single DELETE). Verified: - pnpm --filter @paperclipai/server typecheck (clean) - pnpm --filter @paperclipai/shared typecheck (clean) - vitest run on server/src/__tests__/issue-routine-execution-cancel-permission-route.test.ts (5 tests), issue-routine-owner-cancel-carveout.test.ts (5 tests), issue-agent-mutation-ownership-routes.test.ts (14 tests) — all green. Manual smoke deferred to CTO per acceptance. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…very runbook - README: full user-facing docs covering what the plugin does/does not do, config fields, dry-run workflow, disabling, status mapping, goal-subtree filtering, and PAT setup. - CHANGELOG: initial 0.1.0 entry covering all features and security invariants. - Recovery runbook stored as GLA-816 issue document: audit-log queries, manual GitHub close/delete steps, plugin_state mapping removal, and syncedGoalIds fix workflow. Co-Authored-By: Paperclip <noreply@paperclip.ing>
…tbeat injection Adds native company-level shared_instructions to the Paperclip control plane so founder-policy lives in one canonical record and is prepended to every agent's resolved instructions file at heartbeat time, replacing the filesystem-walk-and-patch model. Schema (migration 0084_shared_instructions.sql): - companies.shared_instructions (nullable text) - agents.shared_instructions_opt_out (boolean, default false) - company_shared_instructions_history (append-only audit log: previous_value, new_value, actor_kind, actor_user_id, request_id, created_at) Rollback statements documented in the migration header. API: - PATCH /api/companies/:id/shared_instructions — board-only write, S1 gate rejects every agent JWT with 403 forbidden_actor_kind. Each successful PATCH appends a history row capturing previous + new values + actor (S2). - GET /api/companies/:id/shared_instructions/history — cursor pagination, reverse-chronological. - PATCH /api/agents/:id — extended to handle sharedInstructionsOptOut with the same S1 board-only gate (S3). - GET /api/agents/:id — includes sharedInstructionsOptOut via the existing full-row select. Heartbeat injection (server/src/services/heartbeat.ts): After the final runtimeConfig is assembled and before adapter.execute, the heartbeat resolves the company's shared_instructions and the agent's opt-out, then calls resolveSharedInstructions to materialize a per-run merged temp file (policy + "\n\n---\n\n" + original AGENTS.md content) under os.tmpdir and swaps runtimeConfig.instructionsFilePath to that path. The temp file is cleaned up in the outer finally block alongside other per-run cleanup. Tests (server/src/__tests__/company-shared-instructions.test.ts): Covers all six acceptance criteria from GLA-873: 1. Policy prepended ahead of role-specific instructions in the merged file. 2. Latest policy used on subsequent runs without restart. 3. Null/empty policy leaves the agent prompt unchanged. 4. Agent JWT (general/cto/cmo/ceo) PATCH returns 403 forbidden_actor_kind. 5. Successful write inserts a history row with previous + new + actor. 6. shared_instructions_opt_out=true skips the prepend for that agent only. Co-Authored-By: Paperclip <noreply@paperclip.ing>
…ation config
When the `repo` field changes, issue a one-time GET /repos/{owner}/{repo}
with the configured PAT before writing the new settings. 401/403/404
responses throw unprocessable(422); the token is redacted from the error
body before it is surfaced. Unchanged repo skips the handshake entirely.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
…stError, or plugin_logs Verifies the redact() fix (landed in GLA-1076) covers all three error-path leak surfaces: the 502 JSON response, plugin_company_settings.lastError, and plugin_logs.meta.error. Co-Authored-By: Paperclip <noreply@paperclip.ing>
Guard against accidental full-company mirror: POST /integrations/github now returns 422 when dryRun is disabled but syncedGoalIds is empty. Matches the safe-default requirement from the GLA-38 threat model. Co-Authored-By: Paperclip <noreply@paperclip.ing>
… CI smoke - watch-and-build.mjs: fs.watch over app/lib/components/pages, 5s debounce, npm run build + pm2 restart asset-library; runs as asset-library-watcher pm2 process - ecosystem.config.js: register asset-library-watcher alongside asset-library - ChunkErrorBanner.tsx: window error listener catches ChunkLoadError, shows "Library is rebuilding — refresh in 10s" overlay with auto-reload countdown - layout.tsx: mount ChunkErrorBanner in root layout - asset-library-smoke.yml: GitHub Actions job triggered on asset-library/** changes; npm ci → npm run build → next start → curl / (200) → curl /asset/:id (200) Co-Authored-By: Paperclip <noreply@paperclip.ing>
Greptile SummaryThis PR bundles four workstreams into a single commit: an asset-library auto-rebuild watcher + CI smoke for the
Confidence Score: 3/5The CI smoke job will fail on every run and the shared-instructions history endpoint can return duplicate rows — fix both before merging. The CI smoke step curls a one-segment URL that has no matching Next.js route, so the smoke job fails unconditionally. The history pagination cursor uses lte(createdAt) alone, which re-includes prior-page rows whenever two audit entries share a timestamp. The remaining changes — migrations, auth bypasses, GitHub sync plugin — are well-guarded and look correct. .github/workflows/asset-library-smoke.yml (broken smoke URL), server/src/services/companies.ts (pagination cursor), scripts/asset-library/ecosystem.config.js (hardcoded paths) Important Files Changed
Prompt To Fix All With AIFix the following 7 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 7
.github/workflows/asset-library-smoke.yml:53-56
**CI smoke URL doesn't match the registered route**
The test expects HTTP 200 from `/asset/ci-smoke-placeholder`, but that path has only one segment. The only asset page in this PR is at `app/asset/[issueId]/[docKey]/`, which requires **two** segments. Next.js App Router will return HTTP 404 for a one-segment path since no `page.tsx` exists at `app/asset/[issueId]/`. The CI step `[ "$code" = "200" ]` will always fail in this state.
### Issue 2 of 7
server/src/services/companies.ts:353-362
**Cursor pagination duplicates rows when timestamps collide**
The cursor filter `lte(createdAt, cursorRow.createdAt)` selects every row whose `createdAt` is at or before the cursor's timestamp. When two rows share the same `createdAt`, rows from the previous page that have the same timestamp but a larger `id` (which sort first under `desc(id)`) are re-included on the next page. Consider narrowing to `(createdAt < cursor.createdAt) OR (createdAt = cursor.createdAt AND id <= cursor.id)` to avoid duplicates.
### Issue 3 of 7
scripts/asset-library/ecosystem.config.js:7-8
**Hardcoded developer-machine absolute path**
`NOTIFIER_ENV_FILE` is set to `/Users/jlqueguiner/.paperclip-worktrees/...` — an absolute path that only exists on one developer's Mac. While the code falls back gracefully when the file is absent, any other team member who runs `pm2 start ecosystem.config.js` will silently get no notifier environment variables loaded. Consider reading from a project-relative path or an env var like `PAPERCLIP_NOTIFIER_ENV_FILE`.
### Issue 4 of 7
scripts/asset-library/ecosystem.config.js:60-62
**Watcher interpreter is hardcoded to macOS Homebrew path**
`interpreter: '/opt/homebrew/opt/node@20/bin/node'` will fail on Linux (CI, Docker, other dev machines). The `asset-library` process handles this via a `PATH` prefix so pm2 can find the right node from `PATH`. The same `PATH`-based approach could be applied to the watcher by using `interpreter: 'node'` and letting the `env.PATH` entry do the resolution.
### Issue 5 of 7
scripts/asset-library/watch-and-build.mjs:27-31
**Source changes arriving during a build are silently dropped**
When `building === true` the debounce callback logs "build in progress — skipping" and returns without rescheduling. Any file change that was debounced and fired while a build was in progress is lost until the next file-system event. A simple fix is to track a `pendingChange` flag and re-arm the build in the `finally` block when a change arrived mid-build.
### Issue 6 of 7
scripts/asset-library/app/layout.tsx:1-33
**Visual change — screenshots needed per CONTRIBUTING.md**
`ChunkErrorBanner.tsx` introduces a full-screen overlay UI component mounted in the root layout. CONTRIBUTING.md requires before/after screenshots (or a short video) for any visual or behavioral change. Please add screenshots demonstrating the banner and auto-reload countdown to the PR description.
### Issue 7 of 7
scripts/asset-library/README.md:1
**PR description is missing required sections from CONTRIBUTING.md**
CONTRIBUTING.md requires every PR to use the project template and include a **Thinking Path**, a **Model Used** section (provider, exact model ID, context window, capabilities), a **Verification** section, a **Risks** section, and a **Checklist**. This PR's description covers Problem/Changes/Acceptance but omits those required sections. Please fill out the full template — especially "Model Used", which is mandatory for all contributors per the guide.
Reviews (1): Last reviewed commit: "feat(GLA-1098): asset-library auto-rebui..." | Re-trigger Greptile |
| - name: Smoke — root (expect 200) | ||
| run: | | ||
| code=$(curl -s -o /dev/null -w "%{http_code}" http://127.0.0.1:7700/) | ||
| [ "$code" = "200" ] || { echo "root: expected 200, got $code" >&2; exit 1; } |
There was a problem hiding this comment.
CI smoke URL doesn't match the registered route
The test expects HTTP 200 from /asset/ci-smoke-placeholder, but that path has only one segment. The only asset page in this PR is at app/asset/[issueId]/[docKey]/, which requires two segments. Next.js App Router will return HTTP 404 for a one-segment path since no page.tsx exists at app/asset/[issueId]/. The CI step [ "$code" = "200" ] will always fail in this state.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/asset-library-smoke.yml
Line: 53-56
Comment:
**CI smoke URL doesn't match the registered route**
The test expects HTTP 200 from `/asset/ci-smoke-placeholder`, but that path has only one segment. The only asset page in this PR is at `app/asset/[issueId]/[docKey]/`, which requires **two** segments. Next.js App Router will return HTTP 404 for a one-segment path since no `page.tsx` exists at `app/asset/[issueId]/`. The CI step `[ "$code" = "200" ]` will always fail in this state.
How can I resolve this? If you propose a fix, please make it concise.| diffSummary: summarizeSharedInstructionsDiff(previousValue, nextValue), | ||
| requestId: input.actor.requestId ?? null, | ||
| }); | ||
|
|
||
| const row = await getCompanyQuery(tx) | ||
| .where(eq(companies.id, id)) | ||
| .then((rows) => rows[0] ?? null); | ||
| if (!row) return null; | ||
| const [hydrated] = await hydrateCompanySpend([row], tx); | ||
| return enrichCompany(hydrated); |
There was a problem hiding this comment.
Cursor pagination duplicates rows when timestamps collide
The cursor filter lte(createdAt, cursorRow.createdAt) selects every row whose createdAt is at or before the cursor's timestamp. When two rows share the same createdAt, rows from the previous page that have the same timestamp but a larger id (which sort first under desc(id)) are re-included on the next page. Consider narrowing to (createdAt < cursor.createdAt) OR (createdAt = cursor.createdAt AND id <= cursor.id) to avoid duplicates.
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/services/companies.ts
Line: 353-362
Comment:
**Cursor pagination duplicates rows when timestamps collide**
The cursor filter `lte(createdAt, cursorRow.createdAt)` selects every row whose `createdAt` is at or before the cursor's timestamp. When two rows share the same `createdAt`, rows from the previous page that have the same timestamp but a larger `id` (which sort first under `desc(id)`) are re-included on the next page. Consider narrowing to `(createdAt < cursor.createdAt) OR (createdAt = cursor.createdAt AND id <= cursor.id)` to avoid duplicates.
How can I resolve this? If you propose a fix, please make it concise.| "/Users/jlqueguiner/.paperclip-worktrees/instances/paperclip-openrunner/secrets/notifier.env"; | ||
|
|
There was a problem hiding this comment.
Hardcoded developer-machine absolute path
NOTIFIER_ENV_FILE is set to /Users/jlqueguiner/.paperclip-worktrees/... — an absolute path that only exists on one developer's Mac. While the code falls back gracefully when the file is absent, any other team member who runs pm2 start ecosystem.config.js will silently get no notifier environment variables loaded. Consider reading from a project-relative path or an env var like PAPERCLIP_NOTIFIER_ENV_FILE.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/asset-library/ecosystem.config.js
Line: 7-8
Comment:
**Hardcoded developer-machine absolute path**
`NOTIFIER_ENV_FILE` is set to `/Users/jlqueguiner/.paperclip-worktrees/...` — an absolute path that only exists on one developer's Mac. While the code falls back gracefully when the file is absent, any other team member who runs `pm2 start ecosystem.config.js` will silently get no notifier environment variables loaded. Consider reading from a project-relative path or an env var like `PAPERCLIP_NOTIFIER_ENV_FILE`.
How can I resolve this? If you propose a fix, please make it concise.| // Debounced 5s so rapid multi-file saves don't stack builds. | ||
| name: "asset-library-watcher", | ||
| cwd: path.resolve(__dirname), |
There was a problem hiding this comment.
Watcher interpreter is hardcoded to macOS Homebrew path
interpreter: '/opt/homebrew/opt/node@20/bin/node' will fail on Linux (CI, Docker, other dev machines). The asset-library process handles this via a PATH prefix so pm2 can find the right node from PATH. The same PATH-based approach could be applied to the watcher by using interpreter: 'node' and letting the env.PATH entry do the resolution.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/asset-library/ecosystem.config.js
Line: 60-62
Comment:
**Watcher interpreter is hardcoded to macOS Homebrew path**
`interpreter: '/opt/homebrew/opt/node@20/bin/node'` will fail on Linux (CI, Docker, other dev machines). The `asset-library` process handles this via a `PATH` prefix so pm2 can find the right node from `PATH`. The same `PATH`-based approach could be applied to the watcher by using `interpreter: 'node'` and letting the `env.PATH` entry do the resolution.
How can I resolve this? If you propose a fix, please make it concise.| if (building) { | ||
| log("build in progress — skipping"); | ||
| return; | ||
| } | ||
| building = true; |
There was a problem hiding this comment.
Source changes arriving during a build are silently dropped
When building === true the debounce callback logs "build in progress — skipping" and returns without rescheduling. Any file change that was debounced and fired while a build was in progress is lost until the next file-system event. A simple fix is to track a pendingChange flag and re-arm the build in the finally block when a change arrived mid-build.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/asset-library/watch-and-build.mjs
Line: 27-31
Comment:
**Source changes arriving during a build are silently dropped**
When `building === true` the debounce callback logs "build in progress — skipping" and returns without rescheduling. Any file change that was debounced and fired while a build was in progress is lost until the next file-system event. A simple fix is to track a `pendingChange` flag and re-arm the build in the `finally` block when a change arrived mid-build.
How can I resolve this? If you propose a fix, please make it concise.| import type { Metadata } from "next"; | ||
| import "./globals.css"; | ||
| import Nav from "./components/Nav"; | ||
| import ChunkErrorBanner from "./components/ChunkErrorBanner"; | ||
|
|
||
| export const metadata: Metadata = { | ||
| title: "Marketing Asset Library", | ||
| description: "Review and approve marketing assets — Paperclip [review-and-ship] queue.", | ||
| }; | ||
|
|
||
| export default function RootLayout({ | ||
| children, | ||
| }: Readonly<{ children: React.ReactNode }>) { | ||
| return ( | ||
| <html lang="en"> | ||
| <body className="antialiased bg-neutral-950 text-neutral-100 min-h-screen overflow-x-hidden"> | ||
| <div className="max-w-7xl mx-auto px-4 sm:px-6 py-8"> | ||
| <header className="mb-6"> | ||
| <h1 className="text-xl font-semibold tracking-tight text-white mb-1"> | ||
| OpenRunner · Marketing Asset Library | ||
| </h1> | ||
| <p className="text-xs text-neutral-500"> | ||
| Internal review surface — <a href="http://127.0.0.1:7700/" className="underline hover:text-neutral-300">http://127.0.0.1:7700/</a> | ||
| </p> | ||
| </header> | ||
| <Nav /> | ||
| <ChunkErrorBanner /> | ||
| <main>{children}</main> | ||
| </div> | ||
| </body> | ||
| </html> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Visual change — screenshots needed per CONTRIBUTING.md
ChunkErrorBanner.tsx introduces a full-screen overlay UI component mounted in the root layout. CONTRIBUTING.md requires before/after screenshots (or a short video) for any visual or behavioral change. Please add screenshots demonstrating the banner and auto-reload countdown to the PR description.
Context Used: CONTRIBUTING.md has a guide for a good PR message ... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/asset-library/app/layout.tsx
Line: 1-33
Comment:
**Visual change — screenshots needed per CONTRIBUTING.md**
`ChunkErrorBanner.tsx` introduces a full-screen overlay UI component mounted in the root layout. CONTRIBUTING.md requires before/after screenshots (or a short video) for any visual or behavioral change. Please add screenshots demonstrating the banner and auto-reload countdown to the PR description.
**Context Used:** CONTRIBUTING.md has a guide for a good PR message ... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.| @@ -0,0 +1,75 @@ | |||
| # Marketing Asset Library (`asset-library`) | |||
There was a problem hiding this comment.
PR description is missing required sections from CONTRIBUTING.md
CONTRIBUTING.md requires every PR to use the project template and include a Thinking Path, a Model Used section (provider, exact model ID, context window, capabilities), a Verification section, a Risks section, and a Checklist. This PR's description covers Problem/Changes/Acceptance but omits those required sections. Please fill out the full template — especially "Model Used", which is mandatory for all contributors per the guide.
Context Used: CONTRIBUTING.md has a guide for a good PR message ... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/asset-library/README.md
Line: 1
Comment:
**PR description is missing required sections from CONTRIBUTING.md**
CONTRIBUTING.md requires every PR to use the project template and include a **Thinking Path**, a **Model Used** section (provider, exact model ID, context window, capabilities), a **Verification** section, a **Risks** section, and a **Checklist**. This PR's description covers Problem/Changes/Acceptance but omits those required sections. Please fill out the full template — especially "Model Used", which is mandatory for all contributors per the guide.
**Context Used:** CONTRIBUTING.md has a guide for a good PR message ... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.|
Moving all my contributions to my private fork. Closing this PR. |
Problem
Asset Library at
:7700broke twice — once from missingnext build, once from stale chunks served after agents pushed new routes without rebuilding.Changes
A. Pre-restart build hook (already existed)
start.shalready guardsnext startwith a stale-build check — no change needed.B. File watcher (
watch-and-build.mjs)app/,lib/,components/,pages/viafs.watch({ recursive: true })npm run build→pm2 restart asset-libraryasset-library-watcherpm2 process (registered inecosystem.config.js)C. Chunk error UI (
ChunkErrorBanner.tsx)window.addEventListener("error")catchesChunkLoadError(and message variants)layout.tsx— covers all routesD. CI smoke (
.github/workflows/asset-library-smoke.yml)scripts/asset-library/**npm ci→npm run build→next start -p 7700→ wait 30s → curl/(200) → curl/asset/ci-smoke-placeholder(200)Acceptance
tsc --noEmitpasses)Closes GLA-1098. Parent: GLA-927.
🤖 Generated with Claude Code