Phase 5 — finish the deferred surfaces & hardening - #7
Merged
Conversation
- Biome (format + lint) folded into `bun run check` (tsc -> biome -> test); `bun run format` to apply. Pragmatic config (TS only, excludes apps/console + fixtures; relaxes no-explicit-any / non-null / param-assign for our style). - lefthook git hooks: pre-commit (biome on staged + typecheck), commit-msg (commitlint conventional), pre-push (bun test); installed via prepare. - commitlint config-conventional (body/footer line-length relaxed for detailed bodies). - one-time `biome check --write` reformatted 53 files (formatting only).
- postgres.integration.test.ts (gated on DATABASE_URL, skipped locally): exercises PostgresJobStore CRUD/checkpoint/trace/listByFactio, PostgresAuditLog + the append-only UPDATE/DELETE triggers, and GraphileQueue migrate/enqueue live - CI `integration` job: postgres:17 service + AURIGA_DOCKER_TESTS=1, runs the habenae + sandbox suites so the Docker sandbox + Postgres/graphile paths run for real on the runner; default `bun run check` stays hermetic
…tyle) - apps/console: Next.js App Router + Tailwind v4 + shadcn-style components (Card/Badge/Table). Pages: dashboard, jobs (tenant-scoped via x-auriga headers), job detail + trace, skill marketplace. Thin HTTP client of apps/api (own types, no server-package imports). - excluded from the root Bun tsc/biome/test gate; verified via `next build` (compiles + typechecks, all 5 routes). Deploys to Vercel; not deployed here.
- packages/chatops: platform-agnostic command parser (list/status/approve/ dashboard/submit/help) + handler dispatching to the control plane, scoped to the caller's factio and through the RBAC submit gate (audit best-effort) - Slack adapter: v0 HMAC signature verification (with replay window) + slash-command parsing + end-to-end dispatch; fully unit-tested (live Slack flow needs a real app) - README: Phase 5 done
|
Warning Review limit reached
More reviews will be available in 38 minutes and 59 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR completes Phase 5 by adding Biome/lefthook/commitlint tooling, a new ChangesNew Features and Tooling
Biome-Driven Formatting Normalization
Sequence Diagram(s)sequenceDiagram
participant Slack
participant handleSlackCommand
participant verifySlackSignature
participant parseSlashCommand
participant parseCommand
participant handleCommand
participant JobStore
Slack->>handleSlackCommand: POST /slack (body, X-Slack-Signature, X-Slack-Request-Timestamp)
handleSlackCommand->>verifySlackSignature: signingSecret, timestamp, body, signature
verifySlackSignature-->>handleSlackCommand: false (stale/tampered)
handleSlackCommand-->>Slack: 401 "invalid signature"
Note over handleSlackCommand,verifySlackSignature: valid path
verifySlackSignature-->>handleSlackCommand: true
handleSlackCommand->>parseSlashCommand: body (URL-encoded)
parseSlashCommand-->>handleSlackCommand: text, userId, userName
handleSlackCommand->>parseCommand: text
parseCommand-->>handleSlackCommand: Command (list/status/approve/submit/...)
handleSlackCommand->>handleCommand: Command + ChatContext
handleCommand->>JobStore: list/get/update
JobStore-->>handleCommand: jobs/job
handleCommand-->>handleSlackCommand: ChatReply { text }
handleSlackCommand-->>Slack: 200 + reply text
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…t time out The first `docker run` pulls oven/bun:1 inside the first test, blowing past the default 5s timeout on a cold CI runner (later tests reuse the warm image and pass in ~440ms). Pull the image at module load, outside any per-test timeout, so each test measures sandbox behavior rather than a one-time image fetch. Export DEFAULT_IMAGE so the test pulls exactly what the driver runs.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
package.json (1)
17-17: ⚡ Quick winFail-open hook installation can silently disable local gates.
lefthook install || truesuppresses real install failures in normal git clones, so pre-commit/commit-msg checks can be missing without notice. Prefer skipping only when.gitis absent, not on actual install errors.Suggested change
- "prepare": "lefthook install || true" + "prepare": "if [ -d .git ]; then lefthook install; fi"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` at line 17, The prepare script in package.json currently uses lefthook install || true, which suppresses all installation errors including legitimate failures that should be visible to developers. Instead of silently ignoring all errors, modify the prepare script to only skip lefthook installation when the .git directory is absent (which occurs in scenarios like npm package installations outside a git repository). This way, actual installation failures in normal git clones will surface and notify developers that their pre-commit/commit-msg hooks may not be properly configured, rather than silently disabling local gates.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 51-53: The setup-bun action in both the check job and integration
job uses bun-version set to latest, making CI non-deterministic and vulnerable
to breaking changes when new Bun versions release. Replace bun-version: latest
with an explicit, pinned version number (e.g., bun-version: 1.x.x where x.x is a
specific release) in both occurrences of the setup-bun action to ensure
reproducible and stable CI behavior across all runs and PRs.
In `@apps/console/app/jobs/`[id]/page.tsx:
- Line 10: In the job page component, the condition checking if a job is missing
needs to be updated to properly return an HTTP 404 status code. Replace the
current return statement that renders a message with a call to the notFound()
function imported from 'next/navigation'. Import notFound from 'next/navigation'
at the top of the file, then in the if (!job) block, call notFound() instead of
returning JSX to ensure the correct HTTP status is sent to the client.
In `@apps/console/app/jobs/page.tsx`:
- Line 33: The job ID in the Link component's href attribute is being
interpolated without URL encoding. If the job ID contains special characters
like forward slashes, question marks, hashes, or percent signs, it will break
the URL routing. Wrap the j.id value with encodeURIComponent() when building the
href in the Link component to properly encode any special characters and ensure
the route works correctly regardless of the ID's content.
In `@apps/console/app/skills/page.tsx`:
- Line 29: The TR component is using s.name as the key, which causes collisions
when multiple versions of the same skill exist in the marketplace. Replace the
key prop value from s.name to a unique identifier that combines the skill name
with its version (such as s.name concatenated with s.version, or if available,
use a unique identifier like s.id that accounts for both name and version
uniqueness). This ensures each row has a distinct key even when skills with
identical names but different versions are present.
In `@apps/console/components/ui/table.tsx`:
- Around line 20-21: The TH component is missing the scope attribute on the th
element, which is required for proper accessibility with screen readers. In the
TH function, add the scope="col" attribute to the th element. This tells
assistive technologies that the header cell applies to its entire column,
improving accessibility for users relying on screen readers.
In `@apps/console/lib/api.ts`:
- Around line 77-82: The dashboard and skills API methods in the api object are
missing the second parameter that enables tenant/role headers (x-auriga-*),
while jobs, job, and trace methods all include true as the second parameter to
enable these headers. Update both the dashboard and skills method calls to
include true as the second parameter (like get<Dashboard>("/dashboard", true)
and get<Skill[]>("/skills", true)) to ensure all console API calls consistently
apply tenant/role headers for proper tenant isolation.
In `@packages/chatops/src/handler.ts`:
- Around line 70-74: The dashboard case is building and returning global
organization-wide aggregates that violate tenant isolation. Modify the
buildDashboard call in the "dashboard" case to include tenant filtering so it
only aggregates data for the requesting tenant. Pass the tenant context (likely
available from ctx) to buildDashboard so that d.totals.jobs, d.totals.tenants,
and d.totals.cost_usd reflect only the current tenant's data, not
organization-wide metrics. This ensures the dashboard command respects the
tenant-scoped isolation contract.
- Around line 57-61: The approve case block currently only validates that the
actor's factio matches the record's factio before approving a job, but lacks
role-based access control enforcement. Before calling ctx.store.update for the
approval in the approve case, add a check to verify that ctx.actor has the
appropriate permission or role to approve jobs, not just that they belong to the
same factio. This ensures only authorized actors can approve jobs, preventing
unauthorized same-factio actors from changing the approval state.
In `@packages/habenae/src/postgres.integration.test.ts`:
- Around line 28-34: The database cleanup using truncate statements is only
performed once in beforeAll, which causes tests to be coupled to execution order
and not isolated. Move the truncate operations (the pool.query calls for
truncating jobs, checkpoints, traces, and audit_events) from the beforeAll hook
to a new beforeEach hook, keeping the pool initialization and migration in
beforeAll. This ensures each test starts with a clean database state regardless
of execution order.
---
Nitpick comments:
In `@package.json`:
- Line 17: The prepare script in package.json currently uses lefthook install ||
true, which suppresses all installation errors including legitimate failures
that should be visible to developers. Instead of silently ignoring all errors,
modify the prepare script to only skip lefthook installation when the .git
directory is absent (which occurs in scenarios like npm package installations
outside a git repository). This way, actual installation failures in normal git
clones will surface and notify developers that their pre-commit/commit-msg hooks
may not be properly configured, rather than silently disabling local gates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8884bd7-af27-4940-94bb-ebd8c22ee741
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (87)
.github/workflows/ci.ymlREADME.mdapps/api/src/app.test.tsapps/api/src/app.tsapps/console/.gitignoreapps/console/README.mdapps/console/app/globals.cssapps/console/app/jobs/[id]/page.tsxapps/console/app/jobs/page.tsxapps/console/app/layout.tsxapps/console/app/page.tsxapps/console/app/skills/page.tsxapps/console/components/ui/badge.tsxapps/console/components/ui/card.tsxapps/console/components/ui/table.tsxapps/console/lib/api.tsapps/console/lib/utils.tsapps/console/next.config.mjsapps/console/package.jsonapps/console/postcss.config.mjsapps/console/tsconfig.jsonbiome.jsoncommitlint.config.jslefthook.ymlpackage.jsonpackages/capella/src/cost.test.tspackages/capella/src/observability.test.tspackages/capella/src/rollup.tspackages/capella/src/tracing.test.tspackages/capella/src/tracing.tspackages/chatops/package.jsonpackages/chatops/src/commands.test.tspackages/chatops/src/commands.tspackages/chatops/src/handler.test.tspackages/chatops/src/handler.tspackages/chatops/src/index.tspackages/chatops/src/slack.test.tspackages/chatops/src/slack.tspackages/chatops/tsconfig.jsonpackages/cli/src/e2e.test.tspackages/cli/src/main.tspackages/core/src/job/spec.test.tspackages/core/src/provider/helpers.tspackages/core/src/skill/crypto.tspackages/core/src/skill/hash.tspackages/core/src/skill/verify.test.tspackages/core/src/skill/verify.tspackages/core/src/trace/trace.test.tspackages/core/src/trace/types.tspackages/currus/src/context.tspackages/currus/src/dispatcher.test.tspackages/currus/src/job-runner.test.tspackages/currus/src/job-runner.tspackages/currus/src/loop.tspackages/currus/src/routing.test.tspackages/currus/src/skills.test.tspackages/currus/src/tools/git.tspackages/currus/src/tools/sandbox-tools.test.tspackages/currus/src/tools/search.tspackages/currus/src/trace-emit.test.tspackages/currus/src/verification.tspackages/evals/src/runner.test.tspackages/habenae/src/audit.test.tspackages/habenae/src/audit.tspackages/habenae/src/dag.tspackages/habenae/src/dashboard.test.tspackages/habenae/src/feedback.test.tspackages/habenae/src/file-store.tspackages/habenae/src/governance.test.tspackages/habenae/src/governance.tspackages/habenae/src/postgres-store.tspackages/habenae/src/postgres.integration.test.tspackages/habenae/src/provider-routing.test.tspackages/habenae/src/scheduler.test.tspackages/habenae/src/scheduler.tspackages/habenae/src/worker.tspackages/provider/src/anthropic.test.tspackages/provider/src/anthropic.tspackages/provider/src/contract.tspackages/provider/src/provider-router.tspackages/provider/src/stub.test.tspackages/sandbox/src/docker.tspackages/sandbox/src/sandbox.test.tspackages/skill-registry/src/bundle.tspackages/skill-registry/src/local-registry.tspackages/skill-registry/src/marketplace.test.tstsconfig.json
Comment on lines
+77
to
+82
| dashboard: () => get<Dashboard>("/dashboard"), | ||
| jobs: () => get<Job[]>("/jobs", true), | ||
| job: (id: string) => get<Job>(`/jobs/${encodeURIComponent(id)}`, true), | ||
| trace: (id: string) => get<Trace>(`/jobs/${encodeURIComponent(id)}/trace`, true), | ||
| skills: () => get<Skill[]>("/skills"), | ||
| }; |
There was a problem hiding this comment.
Apply tenant/role headers to all console API calls.
dashboard and skills skip x-auriga-* headers, while the PR objective says console pages are tenant-scoped via those headers. This can weaken tenant isolation or return unintended data depending on API defaults.
Proposed fix
export const api = {
- dashboard: () => get<Dashboard>("/dashboard"),
+ dashboard: () => get<Dashboard>("/dashboard", true),
jobs: () => get<Job[]>("/jobs", true),
job: (id: string) => get<Job>(`/jobs/${encodeURIComponent(id)}`, true),
trace: (id: string) => get<Trace>(`/jobs/${encodeURIComponent(id)}/trace`, true),
- skills: () => get<Skill[]>("/skills"),
+ skills: () => get<Skill[]>("/skills", true),
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| dashboard: () => get<Dashboard>("/dashboard"), | |
| jobs: () => get<Job[]>("/jobs", true), | |
| job: (id: string) => get<Job>(`/jobs/${encodeURIComponent(id)}`, true), | |
| trace: (id: string) => get<Trace>(`/jobs/${encodeURIComponent(id)}/trace`, true), | |
| skills: () => get<Skill[]>("/skills"), | |
| }; | |
| dashboard: () => get<Dashboard>("/dashboard", true), | |
| jobs: () => get<Job[]>("/jobs", true), | |
| job: (id: string) => get<Job>(`/jobs/${encodeURIComponent(id)}`, true), | |
| trace: (id: string) => get<Trace>(`/jobs/${encodeURIComponent(id)}/trace`, true), | |
| skills: () => get<Skill[]>("/skills", true), | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/console/lib/api.ts` around lines 77 - 82, The dashboard and skills API
methods in the api object are missing the second parameter that enables
tenant/role headers (x-auriga-*), while jobs, job, and trace methods all include
true as the second parameter to enable these headers. Update both the dashboard
and skills method calls to include true as the second parameter (like
get<Dashboard>("/dashboard", true) and get<Skill[]>("/skills", true)) to ensure
all console API calls consistently apply tenant/role headers for proper tenant
isolation.
chatops (real isolation gaps): - approve now enforces the same RBAC gate as submit — a tenant match alone let any same-factio role approve; require the actor's role to be policy-permitted - dashboard command is scoped to the caller's factio (was returning org-wide job/tenant/cost aggregates, violating the handler's tenant-isolation contract) - buildDashboard gains an optional `factio` filter (admin HTTP view stays org-wide) - tests: RBAC-denied approve + tenant-scoped dashboard console (correctness/a11y quick wins): - job detail returns a real 404 via notFound() instead of a 200 with a message - encode job IDs in route hrefs; key skill rows by name@version; th scope="col" ci: pin bun-version to 1.3.12 in both jobs (was `latest` — non-deterministic) test: reset Postgres state per-test (beforeEach) so outcomes don't depend on order Not changed: CodeRabbit flagged the console dashboard/skills calls for missing auth headers — those API routes are intentionally open admin/governance views (gated by the deployment proxy; the API ignores actor headers there), so adding headers would be a no-op.
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.
Phase 5 — finish the deferred surfaces & hardening
Closes out the four items deferred across Phases 0–4. One PR, one commit per track.
Tracks
chore: adopt Biome + lefthook + commitlint) — Biome (format + lint) folded intobun run check; lefthook hooks (pre-commit: biome + typecheck; commit-msg: commitlint; pre-push: tests) installed viaprepare; conventional-commit enforcement. One-timebiome check --writereformatted 53 files.feat(ci): live Docker + Postgres integration job + tests) —DATABASE_URL-gated integration tests exercisePostgresJobStore,PostgresAuditLog+ the append-only triggers, andGraphileQueuelive; a new CIintegrationjob (postgres:17 service +AURIGA_DOCKER_TESTS=1) runs the Docker sandbox + Postgres/graphile paths for real on the runner. The default local gate stays hermetic.feat(console): add Capella web console) —apps/console: Next.js App Router + Tailwind v4 + shadcn-style components (dashboard, jobs, job detail + trace, skills), a thin HTTP client ofapps/apiwith tenant headers. Excluded from the Bun gate; verified vianext build(compiles + typechecks, all 5 routes). Deploys to Vercel; not deployed here.feat(chatops): add Slack ChatOps surface) —packages/chatops: a command parser + handler (list/status/approve/dashboard/submit, tenant-scoped, RBAC submit gate) + a Slack HMAC signature-verifying adapter. Fully unit-tested; the live Slack flow needs a real Slack app.Verification
bun run check— 195 pass / 12 skip, typecheck + Biome clean.apps/console:next buildsucceeds (5 routes).integrationCI job is the live proof for the Docker/Postgres/graphile paths (no local Docker here).Honest scope notes
This finishes the Auriga roadmap: Phases 0–5 across 12 packages + 2 apps.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Infrastructure