fix(agentic-org): KIND e2e supervisor-signal flow + fix the 3 bugs it surfaced - #8958
Conversation
Publishes one supervisor_signal.sent envelope onto the JetStream stream the worker consumes, to exercise the documented observe -> legal menu -> model decides -> legality re-check loop end to end against the in-cluster substrate (Cockroach + NATS + Ollama). Mirrors deploy/provision-nats.ts (host-run against a port-forwarded NATS).
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
…on schema Fixes the three flow bugs surfaced by the KIND e2e supervisor-signal driver. Bug #1 — synthesized reaction actor had no hat authority, so every org command (e.g. CreateDiscussionAnchor) was denied with hat_authority_missing. The authority projection (agentic_org_hat_assignment_authorities) had a reader but no writer, so nothing populated it for the synthetic agent-reaction-<hat>. - Add HatAssignmentAuthorityWriterPort + HatAssignmentAuthorityGrant (ports.ts) - Add createCockroachHatAssignmentAuthorityWriter (idempotent UPSERT) and wire it into the durable state adapters. - In the reaction substrate seeder, grant the synthetic actor its scoped, auditable authority FIRST and unconditionally (before the work-item check), under a real hat (ReactionActorHatId) whose tool bundles cover the action. Bug #2 — re-traced toolTypeForReactionAction: EngineeringManager maps to Prioritize (BacklogAndDefect), which engineering_manager already carries. The supervisor-triage path was already authorized; no fix needed. A mapping-validity test now guards every RequiredHat -> hat -> required-bundle so a future drift can't make a grant vacuous. Bug #3 — the always-on keep-alive / org-cadence loops are started in main() outside the worker process, so their first tick raced the schema migration (which only ran inside the work loop's bootstrap phase) and threw on cold start. Run the migration bootstrap explicitly once, before any loop starts; the worker process takes no bootstrappers (migration is no longer duplicated). Tests: - organization-executor-composition: self-grant path proven without the old stub (grant precedes authorization read); mapping validity. - cockroach-hat-assignment-authority-writer: UPSERT round-trips through reader. - main: loops never tick when the migration bootstrap fails (cold-start gate).
devin-ai-integration
Bot
changed the title
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const connection = await connect({ servers }); | ||
| try { | ||
| const js = jetstream(connection); | ||
| const ack = await js.publish(subject, JSON.stringify(envelope)); |
There was a problem hiding this comment.
🔴 Missing TextEncoder encoding for NATS JetStream publish payload causes runtime failure
The new deploy/drive-supervisor-signal.ts:87 passes JSON.stringify(envelope) (a raw string) to js.publish(), but the existing sibling script deploy/spin-up-task.ts:74 correctly uses new TextEncoder().encode(JSON.stringify(envelope)) to produce a Uint8Array. For @nats-io/jetstream v3+ (package.json specifies ^3.4.0), the publish method's data parameter expects Uint8Array. Since the deploy/ directory is excluded from tsconfig.json's include paths (only packages/**/*.ts and apps/**/*.ts), this type mismatch is not caught by the type checker and will cause a runtime error when the script is executed.
| const ack = await js.publish(subject, JSON.stringify(envelope)); | |
| const ack = await js.publish(subject, new TextEncoder().encode(JSON.stringify(envelope))); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| await input.authorityWriter.grantHatAssignmentAuthority({ | ||
| hatAssignmentId: actor.hatAssignmentId, | ||
| hatId: ReactionActorHatId[action.requiredHat], | ||
| organizationId: action.organizationId, | ||
| projectId: action.projectId, | ||
| ...(action.teamId === undefined ? {} : { teamId: action.teamId }), | ||
| assignedAgentId: actor.agentId, | ||
| state: HatAssignmentAuthorityState.Active, | ||
| updatedAt: input.now(), | ||
| version: 1, | ||
| correlationId: action.triggerEventId, | ||
| causationId: action.triggerEventId, | ||
| traceId: action.triggerEventId, | ||
| }); |
There was a problem hiding this comment.
📝 Info: Authority grant always writes version: 1 — safe only if no other writer updates the row
The grantHatAssignmentAuthority call at agentic-organization/apps/workers/src/organization-executor-composition.ts:142 always passes version: 1. Because the SQL is an unconditional UPSERT (cockroach-hat-assignment-authority-writer.ts:47-61), any pre-existing row with a higher version would be overwritten back to 1. This is safe today because only this code path writes authority rows for synthesized reaction actors (the hat-assignment-reaction-* IDs are deterministic and unique to this path). However, if a future code path updates versions on these rows, the unconditional UPSERT would silently regress them. The comment says "idempotent UPSERT" which accurately describes the current single-writer semantics.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // `string` (not the reader-only statement enum) so the same executor type is | ||
| // shared by the reader and the sibling authority WRITER without a circular | ||
| // import; this mirrors the underlying generic Cockroach executor (name: string). | ||
| name: string; |
There was a problem hiding this comment.
📝 Info: Widened CockroachHatAssignmentAuthoritySqlStatement.name type from enum to string
At agentic-organization/packages/state-cockroach/src/cockroach-hat-assignment-authority-reader.ts:16-19, the name field on CockroachHatAssignmentAuthoritySqlStatement was widened from the reader-only enum (CockroachHatAssignmentAuthorityReaderStatement) to string. The comment explains this avoids a circular import between reader and writer. This is consistent with the underlying generic executor type (CockroachGenericSqlExecutor uses name: string). The trade-off is that the reader loses compile-time enforcement that only known statement names are used, but this mirrors the pattern already used by other executor types in the codebase.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // state, so without this gate tick 1 races the migration and throws, | ||
| // degrading the cold-start cadence until tick 2. Awaiting it here removes that | ||
| // cold-start throw. | ||
| await cockroachMigrationBootstrapper.bootstrap(); |
There was a problem hiding this comment.
📝 Info: Migration failure now surfaces as Degraded exit instead of being retried by the process bootstrap phase
Previously, the migration bootstrapper was passed into createWorkerProcess({ bootstrappers: [...] }) where the process's bootstrap phase would handle it (potentially with process-level retry/error semantics). Now the bootstrapper runs as a bare await cockroachMigrationBootstrapper.bootstrap() at agentic-organization/apps/workers/src/main.ts:350. A failure throws into the catch block at line 425, which logs and returns Degraded. This is the intended behavior (the test at main.test.ts:137-175 explicitly asserts it), but it changes the error-handling semantics: the process no longer has an opportunity to retry the migration within its bootstrap lifecycle. The orchestrator (e.g., Kubernetes) would need to restart the pod.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const row: AuthorityRow = { | ||
| hat_assignment_id: String(parameters[0]), | ||
| hat_id: String(parameters[1]), | ||
| organization_id: String(parameters[2]), | ||
| project_id: String(parameters[3]), | ||
| team_id: parameters[4] === null ? null : String(parameters[4]), | ||
| assigned_agent_id: String(parameters[5]), | ||
| state: String(parameters[6]), |
There was a problem hiding this comment.
📝 Info: Test executor parameter index assumptions are fragile and coupled to SQL column order
The test executor in agentic-organization/apps/workers/test/organization-executor-composition.test.ts:110-117 reconstructs an AuthorityRow from positional parameters indices (e.g., parameters[0] is hat_assignment_id, parameters[5] is assigned_agent_id). These indices are coupled to the column order in the UPSERT SQL at cockroach-hat-assignment-authority-writer.ts:48-61 and the parameter array at cockroach-hat-assignment-authority-writer.ts:28-39. If the column order in the writer changes, these tests would silently pass with wrong field mappings. This is a standard fragility in SQL-level test doubles, not a bug, but worth noting for maintainability.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Stands up the agentic-org worker in KIND with a real cheap Ollama model (
qwen2:0.5b, CPU) as the agent, drives the documented supervisor-signal loop end-to-end, and fixes the three flow bugs that the driver surfaced. The whole chain now succeeds in-cluster:supervisor_signal.sent→ V0 automation rule →CreateSupervisorTriagereaction plan → claimed → Hermes agent run → realPOST /api/chatto in-cluster Ollama (decision kernel re-checks the pick, so the model can't widen the menu) → org command authorized → discussion anchor persisted.This PR contains the KIND e2e driver (
deploy/drive-supervisor-signal.ts) plus the three fixes below.Bug #1 — synthesized reaction actor had no hat authority (org command always denied)
synthesizeActorinventsagent-reaction-<hat>, but the authority projectionagentic_org_hat_assignment_authoritieshad a reader and no writer, so nothing populated it for the synthetic actor. Every org command (e.g.CreateDiscussionAnchor) was denied withhat_authority_missing. The passingdurable-worker-live-integrationtest masked this by stubbing an Active authority row.ReactionActorHatIdmaps each abstractRequiredHatlevel to a concrete org hat whose tool bundles cover the action class the reaction command needs (EngineeringManager→Prioritize,Reviewer→WriteDoc,CSuite/Director/ExecutiveBoard→AssignHat). The grant runs before the work-item existence check so it's never skipped.Bug #2 — re-traced: supervisor-triage was already authorized (no fix needed)
The earlier hypothesis ("supervisor hats can't write docs") was a false positive.
toolTypeForReactionActionmapsEngineeringManager → Prioritize(notWriteDoc), andengineering_manageralready carriesBacklogAndDefect. Added a mapping-validity test so a future drift inReactionActorHatIdcan't make a grant vacuous (every mapped hat must exist and passpreflightHatActionfor its action class).Bug #3 — always-on loops threw on cold-start tick 1
The keep-alive / org-cadence loops are started in
main()outside the worker process, so their first tick raced the schema migration (which only ran inside the work loop's bootstrap phase) and threw, degrading the cold-start cadence until tick 2. Fix: run the migration bootstrap explicitly once before any loop starts; the worker process now takes no bootstrappers (migration is no longer duplicated).Tests
organization-executor-composition.test.ts: rewritten to prove the self-grant path with no stub (grant precedes the authorization read; granted row has the right hat/agent/scope) + mapping validity.cockroach-hat-assignment-authority-writer.test.ts(new): UPSERT round-trips through the reader asActive; null team scope persists.main.test.ts: when the migration bootstrap fails, the loops never tick (cold-start gate).In-cluster verification (KIND, real
qwen2:0.5b)cycle.status="worked",reaction_plan.succeeded_count=1,failed_count=0.hat-assignment-reaction-engineering_manager / engineering_manager / agent-reaction-engineering_manager / active.discussion-anchor-…anchored to the driver'swork-item-<runId>, created by the reaction actor (previouslyhat_authority_missing).POST /api/chat→200in ~4.3s from the worker pod IP.tick:1lane reportedfailureCount:0(no tick-1 throw).Link to Devin session: https://app.devin.ai/sessions/77dac2be40e34088b799b6f1d927dbe3
Requested by: @maximdolphin