Phase 3 — control plane + governance + multi-tenant - #4
Merged
Conversation
- JobSpec.depends_on (DAG); JobRecord.retries; schema regenerated - JobStore.listByFactio across all stores (multi-tenant isolation) - dag.ts: dependencyStatus (ready / failedDeps / pendingDeps; missing dep = failed) + isActive (occupies a concurrency slot) - postgres: retries column + additive ALTER + migration 0003 - tests: tenant isolation, dep readiness/blocking, missing dep, isActive
- Scheduler.drain() runs pending jobs to completion respecting global + per-factio concurrency quotas and the dependency DAG (run only when deps done) - marks dependency-blocked jobs failed (failed/missing dep, or a cycle) instead of hanging; classifies finished jobs done/failed/paused - tests: global cap, per-tenant cap with cross-tenant parallelism, DAG order, blocked-on-failed-dep, cycle → blocked
- RetryPolicy (maxRetries + optional backoffMs(attempt)); scheduler re-enqueues a failed job to pending up to the limit, tracking JobRecord.retries - report.retried records re-enqueue events; exhausted retries end failed - tests: transient fail→retry→succeed, bounded retries stay failed, backoff consulted per attempt
- provider: ModelRouter + staticRouter + reasoningSandwich(strong, fast) - runLoop planModel: step 1 (planning) uses the plan model, later steps the act model - runJob threads planModel; Worker accepts a router and derives plan/act per job - tests: router selection, and step-1=plan / step-2=act via StubProvider.calls
- governance.ts: FactioPolicy (roles, allowed tools/skills) + InMemoryPolicy - submitJob front-door gate (permissions in code): enforces tenant isolation (actor.factio == spec.factio), role membership, tool allowlist; narrows allowed_skills to the tenant set and rejects required skills outside it - tests: permit, cross-tenant deny, unknown factio, role deny, tool deny, skill narrowing, required-skill deny
- core: SkillUsage / SkillStats / SkillUsageSink (runtime→governance channel) - LocalSkillRegistry implements SkillUsageSink: recordUsage aggregates uses/successes/total cost per skill (usage.json); stats() reads them back - Worker feeds per-skill usage after a run (cost attributed across loaded skills, success = job done), via an optional usageSink - tests: registry aggregation; worker feeds usage after a real run
- auriga create <spec.json> create a pending job (for DAGs / batch)
- auriga schedule [opts] drain pending jobs via the Scheduler
(--global, --per-factio, --max-retries)
- auriga list [--factio F] tenant-scoped listing (shows [factio])
- README Phase 3 section; usage updated
- smoke tests: create→list --factio, schedule on empty store
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughWalkthroughPhase 3 adds a full control plane to the auriga project: ChangesPhase 3: Control Plane + Governance + Multi-Tenant Scheduling
Sequence Diagram(s)sequenceDiagram
participant CLI as auriga CLI
participant Scheduler
participant Worker
participant dependencyStatus
participant ModelRouter
participant runJob
participant SkillUsageSink as LocalSkillRegistry
rect rgba(70, 130, 180, 0.5)
note over CLI, Scheduler: schedule command
CLI->>Scheduler: new Scheduler({ store, run: Worker.run, quotas, retry })
CLI->>Scheduler: drain()
end
rect rgba(100, 160, 100, 0.5)
note over Scheduler, dependencyStatus: per-job eligibility check
Scheduler->>dependencyStatus: dependencyStatus(store, record)
dependencyStatus-->>Scheduler: { ready, failedDeps, pendingDeps }
Scheduler->>Scheduler: enforce quotas.global + quotas.perFactio
end
rect rgba(180, 100, 60, 0.5)
note over Worker, SkillUsageSink: job execution + feedback
Scheduler->>Worker: run(jobId)
Worker->>ModelRouter: route(spec)
ModelRouter-->>Worker: { plan, act }
Worker->>runJob: { model, planModel }
runJob-->>Worker: result + trace
Worker->>SkillUsageSink: recordUsage per loadedSkill
end
Scheduler-->>CLI: SchedulerReport
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
migrations/0003_add_retries.sql (1)
2-2: ⚡ Quick winAdd a DB constraint to keep retry counters non-negative.
Line 2 adds the column, but it can still be set to negative values later. Adding a
CHECK (retries >= 0)keeps retry bookkeeping valid across writers.Proposed migration adjustment
alter table jobs add column if not exists retries integer not null default 0; +do $$ +begin + if not exists ( + select 1 + from pg_constraint + where conname = 'jobs_retries_nonnegative' + ) then + alter table jobs + add constraint jobs_retries_nonnegative check (retries >= 0); + end if; +end $$;🤖 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 `@migrations/0003_add_retries.sql` at line 2, The ALTER TABLE statement that adds the retries column to the jobs table is missing a CHECK constraint. Modify the ALTER TABLE statement to include a CHECK constraint that ensures the retries column value is always greater than or equal to zero, preventing negative values from being inserted or updated into this column.packages/habenae/src/postgres-store.ts (1)
80-83: Consider indexing thefactioJSON expression for scheduler/list hot paths.Line 82 filters on
spec->>'factio'; for larger job tables, add an expression index (or generated column + index) to avoid full scans.🤖 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 `@packages/habenae/src/postgres-store.ts` around lines 80 - 83, The listByFactio method filters on the JSON expression spec->>'factio' without database indexing, which causes full table scans on larger job tables. Add a database index on the expression spec->>'factio' in the jobs table to optimize this query path. This can be done either by creating an expression index directly on the JSON extraction or by using a generated column with an index, depending on your PostgreSQL setup and performance requirements.packages/core/src/job/spec.ts (1)
63-64: ⚡ Quick winTighten
depends_onitem validation at parse time.Line 63 currently accepts empty dependency IDs. Rejecting empty strings here avoids late scheduler-time failures from malformed specs.
Proposed schema tweak
- depends_on: z.array(z.string()).optional(), + depends_on: z.array(z.string().min(1)).optional(),🤖 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 `@packages/core/src/job/spec.ts` around lines 63 - 64, The depends_on field in the job spec schema currently accepts empty strings within the dependency ID array, which can cause failures later during scheduling. Tighten the validation by adding a constraint to the z.string() schema element within the z.array() that rejects empty or whitespace-only strings, ensuring all dependency IDs are non-empty at parse time. Use Zod's validation methods (such as min length constraint or a custom validation) to enforce this requirement on each string in the depends_on array.packages/habenae/src/scheduler.test.ts (1)
53-119: ⚡ Quick winAdd a guard-rail test for invalid quota values.
Once options validation is added, include a case for
global <= 0/perFactio <= 0to ensure the scheduler fails fast instead of reclassifying pending jobs as blocked.🤖 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 `@packages/habenae/src/scheduler.test.ts` around lines 53 - 119, Add a new test case after the existing tests in scheduler.test.ts that validates the Scheduler constructor rejects invalid quota configurations. Create a test that attempts to instantiate a Scheduler with quotas where either global or perFactio is set to a value less than or equal to zero, and verify that the scheduler throws an appropriate error or validation exception. This ensures the scheduler fails fast during initialization rather than silently processing jobs with invalid quotas and reclassifying them as blocked.packages/habenae/src/governance.test.ts (1)
35-89: ⚡ Quick winAdd a regression test for cross-factio
depends_onrejection.There’s no case asserting that
submitJob()rejects a spec whose dependency ID belongs to another factio. Adding it would lock in tenant isolation behavior.🤖 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 `@packages/habenae/src/governance.test.ts` around lines 35 - 89, Add a new test case in the governance.test.ts file to verify that submitJob() rejects specs with cross-factio dependencies. The test should create a spec using the spec() helper with a depends_on field that references a job ID from another factio, attempt to submit it with an actor from a different factio, and assert that the call rejects with a PolicyError instance. This will ensure tenant isolation is maintained for job dependencies and prevent jobs from depending on work from other factios.
🤖 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 `@packages/cli/src/main.ts`:
- Around line 207-209: The list function fails to validate that the --factio
flag has a required value. When --factio is provided without a value, flagValue
returns undefined, causing the ternary operator to silently fall back to
store.list() which lists all tenants instead of raising a usage error. Fix this
by checking whether --factio is actually present in the args array and
explicitly validating that it has a non-empty value, throwing an appropriate
error if the flag is provided but lacks its required value. Apply the same
validation logic to all other occurrences where similar flag validation is
needed (also applies to other list operations mentioned in the comment).
In `@packages/habenae/src/governance.ts`:
- Around line 53-87: The submitJob function validates that the actor and spec
share the same factio but does not validate the depends_on field, allowing a job
to depend on another tenant's job. Add a validation check after the existing
actor.factio equality check to iterate through any dependencies in
spec.depends_on (or the appropriate dependency field), retrieve each dependency
from the store, and verify that its factio matches spec.factio. Throw a
PolicyError if any dependency belongs to a different factio, using a descriptive
message similar to the other PolicyError messages in the function.
In `@packages/habenae/src/scheduler.ts`:
- Around line 49-52: The SchedulerOptions in the constructor lacks validation
for numeric limits. The quotas.global and quotas.perFaction values are used
without bounds checks, and when they are zero or negative, jobs cannot start and
get incorrectly marked as blocked or failed in the drain() method. Similarly,
retry.maxRetries lacks validation for being a non-negative integer. Add
validation logic in the constructor to ensure quotas.global and
quotas.perFaction are positive numbers, and that retry.maxRetries is a
non-negative integer, rejecting invalid values before any job state mutations
occur in drain() or related methods that process jobs.
In `@packages/habenae/src/worker.ts`:
- Around line 150-151: The cost calculation for skill attribution on line 150
uses only traceCost(trace) which reflects the current invocation, but when
resuming from a checkpoint, prior-attempt cost data exists in result.usage that
is not being included. Update the total cost calculation to combine both the
current trace cost from traceCost(trace) and any prior attempt costs stored in
result.usage so that the perSkill calculation accurately represents the complete
cost across all attempts, not just the current invocation.
- Around line 149-157: The usageSink.recordUsage() calls in the loop are not
error-handled, which means any failure will reject the entire run() method even
though the job state has already been persisted earlier. Wrap the entire
usageSink recording block (from the if condition checking this.opts.usageSink
through the completion of the for loop that calls recordUsage) in a try-catch
block to make usage recording best-effort, ensuring errors are caught and logged
but do not propagate and fail the job execution.
In `@packages/skill-registry/src/local-registry.ts`:
- Around line 129-133: The read-modify-write sequence in the recordUsage method
(reading with readUsage, modifying the rec object, then writing with writeFile)
is vulnerable to race conditions when multiple concurrent processes access the
same skill's usage.json file. To fix this, implement file locking around the
entire read-modify-write operation to ensure only one process can update the
usage data at a time. Consider using a file locking library or implementing a
lock mechanism that guards the critical section from the readUsage call through
the writeFile call, ensuring atomic updates to the usage statistics.
- Around line 146-151: In the readUsage method, modify the catch block to
differentiate between a missing usage.json file and other read/parse errors.
Only return undefined when the error specifically indicates the file does not
exist (checking the error code for ENOENT). For any other errors such as
permission issues or JSON parse failures, re-throw the error so it surfaces to
the caller rather than silently returning undefined and triggering counter
reinitialization on line 129 that would overwrite valid historical stats.
---
Nitpick comments:
In `@migrations/0003_add_retries.sql`:
- Line 2: The ALTER TABLE statement that adds the retries column to the jobs
table is missing a CHECK constraint. Modify the ALTER TABLE statement to include
a CHECK constraint that ensures the retries column value is always greater than
or equal to zero, preventing negative values from being inserted or updated into
this column.
In `@packages/core/src/job/spec.ts`:
- Around line 63-64: The depends_on field in the job spec schema currently
accepts empty strings within the dependency ID array, which can cause failures
later during scheduling. Tighten the validation by adding a constraint to the
z.string() schema element within the z.array() that rejects empty or
whitespace-only strings, ensuring all dependency IDs are non-empty at parse
time. Use Zod's validation methods (such as min length constraint or a custom
validation) to enforce this requirement on each string in the depends_on array.
In `@packages/habenae/src/governance.test.ts`:
- Around line 35-89: Add a new test case in the governance.test.ts file to
verify that submitJob() rejects specs with cross-factio dependencies. The test
should create a spec using the spec() helper with a depends_on field that
references a job ID from another factio, attempt to submit it with an actor from
a different factio, and assert that the call rejects with a PolicyError
instance. This will ensure tenant isolation is maintained for job dependencies
and prevent jobs from depending on work from other factios.
In `@packages/habenae/src/postgres-store.ts`:
- Around line 80-83: The listByFactio method filters on the JSON expression
spec->>'factio' without database indexing, which causes full table scans on
larger job tables. Add a database index on the expression spec->>'factio' in the
jobs table to optimize this query path. This can be done either by creating an
expression index directly on the JSON extraction or by using a generated column
with an index, depending on your PostgreSQL setup and performance requirements.
In `@packages/habenae/src/scheduler.test.ts`:
- Around line 53-119: Add a new test case after the existing tests in
scheduler.test.ts that validates the Scheduler constructor rejects invalid quota
configurations. Create a test that attempts to instantiate a Scheduler with
quotas where either global or perFactio is set to a value less than or equal to
zero, and verify that the scheduler throws an appropriate error or validation
exception. This ensures the scheduler fails fast during initialization rather
than silently processing jobs with invalid quotas and reclassifying them as
blocked.
🪄 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: 763276c0-a76d-4a18-9d7c-bcdd8b2f2e89
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
README.mdmigrations/0003_add_retries.sqlpackages/cli/src/cli.test.tspackages/cli/src/main.tspackages/core/schema/job.schema.jsonpackages/core/src/job/spec.tspackages/core/src/skill/types.tspackages/currus/src/job-runner.tspackages/currus/src/loop.tspackages/currus/src/routing.test.tspackages/habenae/package.jsonpackages/habenae/src/dag.test.tspackages/habenae/src/dag.tspackages/habenae/src/feedback.test.tspackages/habenae/src/file-store.tspackages/habenae/src/governance.test.tspackages/habenae/src/governance.tspackages/habenae/src/index.tspackages/habenae/src/memory-store.tspackages/habenae/src/postgres-store.tspackages/habenae/src/retry.test.tspackages/habenae/src/scheduler.test.tspackages/habenae/src/scheduler.tspackages/habenae/src/types.tspackages/habenae/src/worker.tspackages/provider/src/index.tspackages/provider/src/router.test.tspackages/provider/src/router.tspackages/skill-registry/src/local-registry.tspackages/skill-registry/src/usage.test.ts
Comment on lines
+129
to
+133
| const rec = (await this.readUsage(name)) ?? { uses: 0, successes: 0, total_cost_usd: 0 }; | ||
| rec.uses += 1; | ||
| if (usage.success) rec.successes += 1; | ||
| if (Number.isFinite(usage.cost_usd)) rec.total_cost_usd += usage.cost_usd; | ||
| await writeFile(join(dir, "usage.json"), `${JSON.stringify(rec, null, 2)}\n`); |
There was a problem hiding this comment.
recordUsage is vulnerable to lost updates under concurrent writes.
Lines 129-133 perform read-modify-write without synchronization. Concurrent workers updating the same skill can overwrite each other, dropping uses/successes/total_cost_usd increments.
🤖 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 `@packages/skill-registry/src/local-registry.ts` around lines 129 - 133, The
read-modify-write sequence in the recordUsage method (reading with readUsage,
modifying the rec object, then writing with writeFile) is vulnerable to race
conditions when multiple concurrent processes access the same skill's usage.json
file. To fix this, implement file locking around the entire read-modify-write
operation to ensure only one process can update the usage data at a time.
Consider using a file locking library or implementing a lock mechanism that
guards the critical section from the readUsage call through the writeFile call,
ensuring atomic updates to the usage statistics.
Major: - governance: reject cross-tenant depends_on at submit (no cross-factio gating/leak) - scheduler: validate quotas (>=1) + maxRetries (>=0) in the constructor - worker: skill-usage feedback is best-effort (Promise.allSettled, warns) so it can't fail an already-persisted job; cost attribution uses result.usage (cumulative across resumes) instead of this-invocation trace cost - skill-registry: serialize recordUsage per instance (no lost updates in-process); readUsage only swallows ENOENT, surfaces corruption/permission errors - cli: `list --factio` with no value is a usage error; flagValue won't consume a following --flag as a value Nitpicks: - spec.depends_on items must be non-empty (schema regenerated) - migration 0003 + SCHEMA_SQL: retries CHECK (>= 0) + jobs_factio_idx index - tests: invalid-quota rejection, cross-tenant dependency rejection
Contributor
Author
|
Addressed the review in d045969 — all 7 actionable + the nitpicks: Major
Nitpicks
Suite: 166 pass / 6 skip, typecheck clean. |
This was referenced Jun 18, 2026
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 3 — control plane + full governance + multi-tenant
Builds on Phases 0–2 (on
main). Seven focused commits.What's new
@auriga/core,@auriga/habenae) —JobSpec.depends_on;JobRecord.retries;JobStore.listByFactio(tenant isolation);dependencyStatus(ready / blocked / waiting; missing dep = failed).@auriga/habenae) —Scheduler.drain()runs pending jobs respecting global + per-tenant concurrency quotas and the dependency DAG; marks dependency-blocked jobs (failed/missing dep, cycles) failed instead of hanging.RetryPolicy(maxRetries + backoff); failed jobs are re-enqueued up to the limit, tracked inJobRecord.retries.@auriga/provider+currus+ worker) —ModelRouter+reasoningSandwich(strong, fast): a strong model plans (step 1), a fast model executes; selected per job by the Worker.@auriga/habenae) —submitJobenforces tenant isolation (actor.factio == spec.factio), role membership, and a tool allowlist; narrowsallowed_skillsto the tenant's permitted set and rejects required skills outside it (permissions in code).@auriga/core,skill-registry, worker) —SkillUsageSink; the registry aggregates per-skill uses/successes/cost; the Worker feeds usage back after each run (runtime → governance).create,schedule(--global/--per-factio/--max-retries),list --factio.Verification
bun run check— 164 pass / 6 skip, typecheck clean.Notes
0002/0003plus an idempotentmigrate()(add column if not exists); verified live once Docker/Postgres is up (tested here via in-memory/file stores).🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
depends_on(DAG-style gating)createandschedulecommands, pluslist --factiotenant-scoped filtering (withschedulereporting run outcomes)