Promote dev → main: A2A routing + chokepoint patterns #4 & #5 + hardening (v0.7.23 candidate)

[mabry1985]

Promotion of 41 commits accumulated on dev since v0.7.22 (#466).

Highlights

A2A correctness (#471 + earlier)

• fix(a2a-server): default to [ava] target + sanitize upstream HTML errors (closes #471) #473 — /a2a defaults to [ava] target when no explicit routing hint (was misrouting to protoBot); HTML error pages from misrouted upstreams now sanitized before becoming reply text.
• fix(a2a): agent card advertises canonical A2A endpoint, not the dashboard URL (closes #461) #462 — agent card advertises canonical A2A endpoint instead of dashboard URL.

GOAP chokepoint-invariant pattern — 5 instances now landed

This promotion completes the pattern-cluster around enforcing invariants at single chokepoints:

• feat(goap): per-action cooldown in ActionDispatcher (closes #437) #452 (fix(goap): action.fleet_incident_response and alert.fleet_agent_stuck need dedup/cooldown — fired duplicate INC-003 through INC-009 in 30s #437) — per-action cooldown in ActionDispatcher
• feat(goap): pre-dispatch target registry guard (closes #444) #455 (fix(goap): pre-dispatch registry check missing — dispatches to unregistered agents (auto-triage-sweep, user) cause 100% failure rate #444) — pre-dispatch target-registry guard
• fix(fleet-health): filter outcomes from synthetic actors via registry whitelist (closes #459) #460 (fix(fleet-health): don't attribute outcomes to synthetic/plugin actors — pr-remediator, auto-triage-sweep show as 'stuck agents' #459) — fleet-health filter for synthetic actors
• fix(pr-remediator): never auto-close promotion PRs on decomposable verdict #469 (fix(pr-remediator): "decomposable" verdict mis-applied to promotion PRs — auto-closes legitimate dev→main work #465) — destructive-verdict guard: pr-remediator never auto-closes promotion PRs on `decomposable` verdict
• fix(pr-remediator): Number.isFinite guard on ttlMs — Infinity/NaN RangeError (#467 finding #2) #470 (follow-up(coderabbit): 6 hardening notes from PR #466 review #467 feat(bus): extensible HITL plugin + /api/projects + /api/agents endpoints #2) — `Number.isFinite` guard on `ttlMs` (Infinity/NaN → no RangeError)

Hardening cleanup (#467 follow-ups from CodeRabbit on #466)

• fix(467): rollcall path + INTERNAL_HOST env doc + validator after workspace plugins #474 — three remaining items:
  • rollcall.md hard-coded path → relative
  • README env-table row for `WORKSTACEAN_INTERNAL_HOST`
  • Startup wiring validator now runs AFTER `loadWorkspacePlugins()` AND on every `config.reload` (was missing both)
• 9792b0c — `pr-remediator-skill-executor` gated on GitHub creds (matches the remediator's own gate; closes a silent-failure path)

Ceremony / GOAP loop hygiene

• fix(ceremony): honor enabled:false on initial load (closes #453) #456 (fix(ceremony-loader): enabled: false not honored on initial load — disabled ceremonies still fire on cron #453) — ceremony honors enabled: false on initial load
• fix(ceremonies): route 4 ceremonies to right skill owner, disable 3 broken #454 — 4 ceremonies routed to right skill owner; 3 broken ceremonies disabled
• fix(goap): disable fleet_agent_stuck loop — fires every cycle, no dedup #436, fix(goap): disable 4 more action.* loops — same no-cooldown bug as #436 #451 — disable broken action.* and fleet_agent_stuck loops missing dedup

Fleet

• feat(agents): register protomaker A2A agent #433 — protoMaker A2A agent registered (closes part of fix(skill-dispatcher): wire remaining 9 actions surfaced by #427 validator #430)
• fix(pr-remediator): wire 5 GOAP-dispatched skills #432 — pr-remediator wires 5 GOAP-dispatched skills + honors hitlPolicy
• feat(ceremony): wire ceremony.security_triage + ceremony.service_health_discord #431 — ceremony.security_triage + ceremony.service_health_discord wired

Other

• follow-up(coderabbit): 6 hardening notes from PR #466 review #467 fix(plane): carry Plane context through A2A hop via pending map #4 — already shipped (9792b0c)
• Back-merge main → dev landed via chore: back-merge main → dev #468 to keep merge-base sane

What's NOT in this promotion

• feat(a2a-server): emit intermediate progress events + durable push-notification store #472 — A2A streaming progress events + durable push-notification store. Real engineering scope (new bus topic family + persistence layer); deferred to its own PR.
• follow-up(coderabbit): 6 hardening notes from PR #466 review #467 Milestone 1: OnboardingPlugin Core + Config Hot-Reload #6 — unspecified sixth CodeRabbit finding from Promote dev → main: agent card + fleet-health filter + ceremony fixes (v0.7.22 candidate, re-cut) #466 (will identify later if it surfaces).

Open issues this closes

`closes #426 #427 #428 #430 #436 #437 #444 #453 #459 #465 #467`

(#471 already closed by #473's merge to dev; will re-close on main once promoted.)

Test plan

• `bun test` clean on dev (1054/1054 pass as of fix(467): rollcall path + INTERNAL_HOST env doc + validator after workspace plugins #474)
• Container deploy to `:dev` confirmed running before promotion
• HITL approval — Josh
• Post-merge: confirm `publish-image.yml` runs and `:main` tag updates
• Watchtower auto-pull picks up new image

Notes for the human gate

• Topology note: the back-merge in chore: back-merge main → dev #468 was force-merged after CodeRabbit timed out, so dev's tip is a squash-equivalent of `14e2045` rather than carrying it as an ancestor commit. Content is identical. Future merge-base may compute slightly oddly but the diff is correct.
• Version bump (`0.7.22 → 0.7.23`) not yet in this PR — happy to add as a separate commit on dev or fold into the promote workflow per your preference.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added environment variable support for external URL configuration and internal host settings.
  • Enhanced PR handling with automatic detection of promotion-style pull requests.
  • Implemented human-in-the-loop (HITL) policy support with configurable timeout behavior.
  • Improved downstream agent error responses with HTML error detection and sanitization.

• Documentation

  • Updated configuration documentation with new environment variables.
  • Revised agent rollcall instructions.

• Tests

  • Added comprehensive test coverage for new PR remediation and agent communication features.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds three new executor plugins (alert-skill, ceremony-skill, pr-remediator) to handle previously unexecuted GOAP skills, implements startup and live wiring validation to catch missing executors, enhances PR remediator with promotion-PR detection and HITL policy forwarding, adds HTML error sanitization in A2A server, and updates GitHub API responses to include head branch references.

Changes

Cohort / File(s)	Summary
Documentation & Configuration .claude/commands/rollcall.md, README.md, workspace/agents/ava.yaml	Updated rollcall command to use relative path and documented new environment variables (WORKSTACEAN_PUBLIC_BASE_URL, WORKSTACEAN_INTERNAL_HOST). Renamed Ava's web search tool from web_search to searxng_search and added promotion-PR rule to diagnose verdict logic.
Plugin Registry & Wiring Validation src/index.ts	Registered three new executor plugins: alert-skill-executor, ceremony-skill-executor, and pr-remediator-skill-executor. Updated existing plugins to accept executorRegistry. Added fail-loud validateActionExecutors validation pass at startup and on every config reload, with strict mode gating via WORKSTACEAN_STRICT_WIRING.
PR Remediator Enhancement lib/plugins/pr-remediator.ts	Extended PR entry with optional headRef field. Added promotion-PR detection via isPromotionPr() checking protected branches and title prefixes. Implemented HITL policy extraction from meta.hitlPolicy with ttlMs and onTimeout validation. Added escalation flow for promotion PRs instead of decomposable close+split.
PR Remediator Tests src/pr-remediator.test.ts	Updated test fixtures with headRef support. Added comprehensive test coverage for decomposable verdict handling including promotion-PR guard scenarios and HITL policy propagation with fallback behavior.
A2A Server Error Handling src/api/a2a-server.ts, src/api/a2a-server.test.ts	Added HTML error detection and sanitization helpers to identify upstream error pages. Set default targets: ["ava"] when metadata omits targets. Sanitize and log HTML errors instead of passing raw markup to downstream callers. Added four new Phase 7 integration tests validating error handling and target routing.
GitHub API Response Shape src/api/github.ts	Extended PR pipeline response to include headRef (branch name) alongside existing headSha and baseRef fields. Updated internal PrListItem typing to reflect expanded head object structure in GitHub response payload.

Sequence Diagram

sequenceDiagram
    participant Startup as Startup Process
    participant PluginMgr as Plugin Manager
    participant Registry as Executor Registry
    participant Validator as Wiring Validator
    participant Config as actions.yaml

    Startup->>PluginMgr: loadWorkspacePlugins()
    PluginMgr->>Registry: register alert-skill-executor
    PluginMgr->>Registry: register ceremony-skill-executor
    PluginMgr->>Registry: register pr-remediator-skill-executor
    PluginMgr-->>Startup: plugins loaded

    Startup->>Validator: validateActionExecutors(throwOnUnwired)
    Validator->>Config: scan actions.yaml for all skills
    Validator->>Registry: check each skill has executor
    alt All executors wired
        Validator-->>Startup: validation passed
    else Missing executors (strict=1)
        Validator-->>Startup: throw error, startup fails
    else Missing executors (strict=0)
        Validator-->>Startup: log warnings, continue
    end

    Startup->>Startup: listen CONFIG_RELOAD events
    note over Startup: On reload, rerun validator<br/>to catch newly added/removed actions

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

• fix(skill-dispatcher): 6 GOAP-wired alert/triage skills have no executor — dispatches silently dropped #426 — Directly addresses the critical gap of missing executors for 6 GOAP alert/triage skills by adding alert-skill-executor, ceremony-skill-executor, and pr-remediator-skill-executor plugins plus startup/live wiring validation.
• follow-up(coderabbit): 6 hardening notes from PR #466 review #467 — The changes implement several hardening points from the follow-up issue: relative rollcall paths, HITL policy ttlMs validation, pr-remediator executor gating, and wiring-validator integration.
• M6: Projects and CI — project cards with CI health bars and PR pipeline #106 — The executor registry wiring and runtime validation address prevention of routing to non-existent agents and ensure agent-rollcall consistency.

Possibly related PRs

• feat(ava): expand helm toolset, wire GOAP skills, fix ceremony disable bug #415 — Both PRs modify agent configuration and DeepAgent documentation/runtime behavior (searxng_search tool rename, promotion-PR verdict logic in Ava).

Poem

🐰 Three executors hop into the registry,
Each skill finds its handler—no mystery!
The validator stands guard at startup's door,
"Show me your wiring, or we're closing this floor!"
GOAP goals now dispatch without a care,
The remediation flywheel spins through the air! 🎯

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: promoting dev to main with A2A routing, chokepoint patterns, and hardening for v0.7.23.
Linked Issues check	✅ Passed	The PR addresses #426's requirements: wires missing alert/triage executors (alert-skill-executor, ceremony-skill-executor, pr-remediator-skill-executor plugins), implements fail-loud validation (startup wiring validator runs after loadWorkspacePlugins and on config.reload), and adds actionable error handling for HTML errors and misrouting.
Out of Scope Changes check	✅ Passed	All changes are within scope: A2A routing improvements, GOAP executor wiring, validation infrastructure, promotion-PR safeguards, and hitlPolicy support directly address the linked issue objectives.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch dev

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 10

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/plugins/CeremonyPlugin.ts (1)

503-525: ⚠️ Potential issue | 🟠 Major

Hot-reload still misses same-size edits.

This path only reloads when the file size changes. A schedule/target/notify-channel edit with the same byte length never reaches _reloadChangedCeremonies(), so the new enable/disable hot-reload behavior can still be skipped until restart.

Possible fix

-import { existsSync, mkdirSync, readdirSync, copyFileSync } from "node:fs";
+import { existsSync, mkdirSync, readdirSync, copyFileSync, statSync } from "node:fs";
...
-        const stat = Bun.file(filePath).size;
+        const stat = statSync(filePath);
         const existing = this.fileSnapshots.get(filePath);
+        const snapshot = { mtime: stat.mtimeMs, size: stat.size };

         // New or changed file
-        if (!existing || existing.size !== stat) {
-          this.fileSnapshots.set(filePath, { mtime: Date.now(), size: stat });
+        if (!existing || existing.size !== snapshot.size || existing.mtime !== snapshot.mtime) {
+          this.fileSnapshots.set(filePath, snapshot);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugins/CeremonyPlugin.ts` around lines 503 - 525, The current hot-reload
only compares file size (stat) so same-size edits are ignored; change the
change-detection in the watcher to compare a file modification timestamp as well
(e.g., use Bun.file(filePath).mtime or stat.mtimeMs) and store that alongside
size in this.fileSnapshots; when deciding to reload, treat the file as changed
if either size or mtime differs, update the snapshot entry ({ mtime, size }) and
then run the same new-file vs changed-file logic (loader.loadGlobal(),
registerCeremony(), or _reloadChangedCeremonies()) so edits that preserve byte
length still trigger reloads.

🧹 Nitpick comments (4)

src/pr-remediator.test.ts (1)

1007-1142: Add one bus-level hitlPolicy test for the real merge-ready flow.

These assertions only cover _extractHitlPolicy() and _emitHitlApproval() in isolation. They won't catch regressions in the production path (world.state.updated → _selfDispatchRemediation() → _handleMergeReady()), which is where the new policy currently gets dropped.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/pr-remediator.test.ts` around lines 1007 - 1142, The test suite lacks an
integration-level test that exercises the real production path so hitlPolicy
isn't dropped between world.state.updated → _selfDispatchRemediation() →
_handleMergeReady(); add a new test that installs PrRemediatorPlugin on an
InMemoryEventBus, captures "hitl.request.pr.merge.#", emits the real merge-ready
trigger (the same message format used by
world.state.updated/pr.remediate.merge_ready) with payload.meta.hitlPolicy set
(e.g., { ttlMs: 1_800_000, onTimeout: "approve" }), waits/flushes microtasks,
and asserts the captured HITLRequest contains the same ttlMs and onTimeout
values to ensure _extractHitlPolicy and the production flow
(_selfDispatchRemediation, _handleMergeReady, _emitHitlApproval) propagate the
policy end-to-end.

src/plugins/agent-fleet-health-plugin.ts (1)

327-334: Consider caching the known-agent set for high-outcome-volume scenarios.

_isRegisteredAgent() calls list() and iterates on every outcome. While acceptable for current scale, if outcome volume grows significantly, consider caching the known-agent names and invalidating on registry changes.

♻️ Optional: Cache known agents

+  private cachedKnownAgents: Set<string> | null = null;
+
   private _isRegisteredAgent(actor: string): boolean {
     if (!this.executorRegistry) return true;
-    const known = this.executorRegistry
-      .list()
-      .map(r => r.agentName)
-      .filter((n): n is string => typeof n === "string" && n.length > 0);
-    return known.includes(actor);
+    if (!this.cachedKnownAgents) {
+      this.cachedKnownAgents = new Set(
+        this.executorRegistry
+          .list()
+          .map(r => r.agentName)
+          .filter((n): n is string => typeof n === "string" && n.length > 0)
+      );
+    }
+    return this.cachedKnownAgents.has(actor);
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugins/agent-fleet-health-plugin.ts` around lines 327 - 334, The
_isRegisteredAgent method repeatedly calls this.executorRegistry.list() and
builds an array per outcome; change it to maintain a cached Set<string> of known
agent names (e.g., this._knownAgents) and have _isRegisteredAgent check that Set
(fall back to calling list() on first access). Update the cache whenever the
registry changes by listening for registry change events or by adding a small
helper method (e.g., refreshKnownAgents) that calls
this.executorRegistry.list(), rebuilds the Set, and is invoked on registry
update points; ensure any null/undefined checks for this.executorRegistry remain
and the cache is invalidated or refreshed when executors are added/removed.

src/planner/__tests__/end-to-end-loop.test.ts (1)

142-152: Cooldown test should include a post-outcome re-violation to isolate dispatcher behavior.

Right now, burst suppression can be influenced by planner inFlightGoals. Add one more violation after the first dispatch/outcome settles to prove meta.cooldownMs alone blocks re-dispatch.

Proposed test hardening

@@
-    // Spam-publish 100 violations of the same goal.
+    // Spam-publish 100 violations of the same goal.
     for (let i = 0; i < 100; i++) {
       bus.publish("world.goal.violated", makeViolation("fleet.no_skill_orphaned", `corr-${i}`));
     }
     await new Promise((r) => setTimeout(r, 50));
 
+    // Re-violate after the first dispatch/outcome cycle settles.
+    // This isolates ActionDispatcher cooldown from planner in-flight suppression.
+    bus.publish("world.goal.violated", makeViolation("fleet.no_skill_orphaned", "corr-after-settle"));
+    await new Promise((r) => setTimeout(r, 20));
+
     // Exactly 1 dispatch reaches the executor; the cooldown drops the rest.
     // The planner's inFlightGoals + ActionDispatcher's cooldown both contribute,
     // but the cooldown is what holds after the outcome clears inFlightGoals.
     expect(skillRequests).toHaveLength(1);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/planner/__tests__/end-to-end-loop.test.ts` around lines 142 - 152, The
test should explicitly re-publish one more violation after the first dispatch
and its outcome have settled to ensure suppression is due to ActionDispatcher's
meta.cooldownMs rather than the planner's inFlightGoals; modify the end-to-end
test around the loop that publishes 100 violations (using bus.publish(...,
makeViolation(...))) to wait for the first dispatch/outcome to complete (i.e.,
ensure skillRequests has the single request and any outcome handlers have run)
and then call bus.publish once more with a new correlation id, and finally
assert that skillRequests remains length 1, referencing the existing symbols
bus.publish, makeViolation, skillRequests, inFlightGoals, and the dispatcher
meta.cooldownMs/ActionDispatcher to locate the right spot.

src/api/agent-card.ts (1)

51-53: Harden public-base normalization before composing the card URL.

At Line 52, trimming only one trailing slash can still leave malformed output for values with whitespace or multiple trailing slashes.

Suggested tweak

-  const publicBase = (process.env.WORKSTACEAN_PUBLIC_BASE_URL ?? "").replace(/\/$/, "");
+  const publicBase = (process.env.WORKSTACEAN_PUBLIC_BASE_URL ?? "")
+    .trim()
+    .replace(/\/+$/, "");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/api/agent-card.ts` around lines 51 - 53, The resolveA2aUrl function
currently only strips a single trailing slash from the env var (publicBase) and
doesn't trim whitespace, so normalize the value more robustly by trimming
surrounding whitespace and removing any number of trailing slashes before
composing the URL; update the publicBase assignment in resolveA2aUrl to use
.trim() and .replace(/\/+$/,"") on process.env.WORKSTACEAN_PUBLIC_BASE_URL (keep
the existing empty-string fallback) and preserve the existing early-return
behavior if publicBase is empty, then return `${publicBase}/a2a`.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@lib/plugins/pr-remediator.ts`:
- Around line 1227-1239: The ttlMs extracted in _extractHitlPolicy can be an
extremely large finite number and later cause new Date(Date.now() + ttlMs) to
throw RangeError; fix by defining a MAX_DATE_MS constant (e.g. the max safe
epoch in ms) and cap ttlMs during extraction in _extractHitlPolicy (set
out.ttlMs = Math.min(p.ttlMs, MAX_DATE_MS - Date.now()) while ensuring
non-negative), and also re-validate/cap ttlMs at the consumption point before
calling new Date(...) to guard again against overflow; reference the
_extractHitlPolicy method and the place where ttlMs is used to compute the
deadline so both locations apply the same MAX_DATE_MS cap.

In `@src/api/a2a-server.ts`:
- Around line 114-127: The current logic unconditionally sets targets = ["ava"]
when metadata.targets is empty, which overrides routing based on a provided
skillHint; change the branch in the handler (the explicitTargets / targets
logic) to only set targets = ["ava"] when there are no explicitTargets AND
metadata.skillHint (or equivalent routing hint field) is falsy — if a skillHint
exists leave targets undefined so skill-based routing can proceed; apply the
same fix to the other occurrence around the similar targets defaulting (lines
~243-246) to ensure we don't hard-pin Ava when a routing hint is present.
- Around line 49-74: The current looksLikeHtmlError and sanitizeHtmlError are
too permissive and leak body content; change looksLikeHtmlError to detect
explicit HTTP-error signatures only (case-insensitive exact phrases like "404
Not Found", "500 Internal Server Error", common server banners such as "nginx/",
"apache/", "cloudflare", and regexes matching "<title>\\d{3}" or "cannot POST /"
with word boundaries) rather than any occurrence of "404 not found" or starting
with "<!doctype"/"<html>". Update sanitizeHtmlError to never return upstream
body text to clients — return a fully generic message like "Downstream agent
returned an HTTP error; see server logs for details" and write the full raw
payload to the server log (use the existing logger used elsewhere in this file)
for debugging. Apply the same tightened logic where these helpers are reused
(the other occurrence referenced in the review).

In `@src/planner/__tests__/validate-action-executors.test.ts`:
- Around line 148-156: The test is mocking the bus instead of using the real
in-memory bus, so replace the fake bus object used in the
validateActionExecutors(actions, executors, { bus: bus as never }) call with the
actual in-memory bus instance used by your codebase (the same Bus implementation
exercised by other tests) so publish/subscribe go through the real transport;
locate the mocked symbols (published array, bus.subscribe, bus.unsubscribe,
bus.publish, BusMessage) and remove the mock(...) wrappers, instantiate the real
in-memory bus, pass it into validateActionExecutors, and assert published
messages via the real bus API to keep the test aligned with other *.test.ts
files and avoid mocking the transport.

In `@src/plugins/__tests__/alert-skill-executor-plugin.test.ts`:
- Around line 11-29: The test defines a local makeBus stub
(functions/published/subscribe/publish/topics/unsubscribe) which duplicates
event-bus behavior; replace this local makeBus with the project's shared
in-memory bus used by other tests (import the shared in-memory bus factory or
singleton and use it instead of makeBus) so the test uses the canonical bus
implementation and complies with the no-mocks in-memory-bus rule; update
references to published, subscribe, publish, topics and unsubscribe to use the
imported bus API.

In `@src/plugins/__tests__/ceremony-skill-executor-plugin.test.ts`:
- Around line 11-29: Replace the hand-rolled makeBus stub with the shared
InMemoryEventBus used across plugin tests: import InMemoryEventBus and
instantiate it in place of makeBus(), then update references that relied on the
local published array or custom methods to use InMemoryEventBus's
publish/subscribe/unsubscribe and any provided inspection helpers (e.g.,
getPublished or events history) so tests exercise the real in-memory
publish/subscribe contract; remove the makeBus function and adapt assertions to
the InMemoryEventBus API in the ceremony-skill-executor-plugin test.

In `@src/plugins/__tests__/pr-remediator-skill-executor-plugin.test.ts`:
- Around line 13-30: Replace the bespoke makeBus() test stub with the shared
InMemoryEventBus: import or require InMemoryEventBus in the test file and
instantiate it instead of calling makeBus(), then use the InMemoryEventBus
instance's subscribe, unsubscribe, publish and topics APIs (and its typed
BusMessage usage) so tests exercise the real in-memory semantics; remove the
makeBus() function and update any references in
pr-remediator-skill-executor-plugin.test.ts to call the InMemoryEventBus
instance methods (e.g., subscribe/unsubscribe/publish/topics) so the test uses
the canonical event-bus behavior.
- Around line 179-204: The test currently only checks that the topic
"pr.remediate.update_branch" exists and that
ExecutorRegistry.resolve("action.pr_update_branch") returns an executor, but it
never verifies that PrRemediatorPlugin actually received the dispatched message;
update the test to observe the remediator handling by subscribing or spying on
the bus for the remediator topic before calling executor.execute (e.g., use
InMemoryEventBus.subscribe("pr.remediate.update_branch", ...) or capture
bus.publish calls) and assert that the subscriber was invoked or that an
expected remediator side-effect occurred after executor.execute completes;
reference the existing InMemoryEventBus, PrRemediatorPlugin.install/uninstall,
ExecutorRegistry.resolve("action.pr_update_branch"), and executor.execute call
to locate where to add the observable assertion.

---

Outside diff comments:
In `@src/plugins/CeremonyPlugin.ts`:
- Around line 503-525: The current hot-reload only compares file size (stat) so
same-size edits are ignored; change the change-detection in the watcher to
compare a file modification timestamp as well (e.g., use
Bun.file(filePath).mtime or stat.mtimeMs) and store that alongside size in
this.fileSnapshots; when deciding to reload, treat the file as changed if either
size or mtime differs, update the snapshot entry ({ mtime, size }) and then run
the same new-file vs changed-file logic (loader.loadGlobal(),
registerCeremony(), or _reloadChangedCeremonies()) so edits that preserve byte
length still trigger reloads.

---

Nitpick comments:
In `@src/api/agent-card.ts`:
- Around line 51-53: The resolveA2aUrl function currently only strips a single
trailing slash from the env var (publicBase) and doesn't trim whitespace, so
normalize the value more robustly by trimming surrounding whitespace and
removing any number of trailing slashes before composing the URL; update the
publicBase assignment in resolveA2aUrl to use .trim() and .replace(/\/+$/,"") on
process.env.WORKSTACEAN_PUBLIC_BASE_URL (keep the existing empty-string
fallback) and preserve the existing early-return behavior if publicBase is
empty, then return `${publicBase}/a2a`.

In `@src/planner/__tests__/end-to-end-loop.test.ts`:
- Around line 142-152: The test should explicitly re-publish one more violation
after the first dispatch and its outcome have settled to ensure suppression is
due to ActionDispatcher's meta.cooldownMs rather than the planner's
inFlightGoals; modify the end-to-end test around the loop that publishes 100
violations (using bus.publish(..., makeViolation(...))) to wait for the first
dispatch/outcome to complete (i.e., ensure skillRequests has the single request
and any outcome handlers have run) and then call bus.publish once more with a
new correlation id, and finally assert that skillRequests remains length 1,
referencing the existing symbols bus.publish, makeViolation, skillRequests,
inFlightGoals, and the dispatcher meta.cooldownMs/ActionDispatcher to locate the
right spot.

In `@src/plugins/agent-fleet-health-plugin.ts`:
- Around line 327-334: The _isRegisteredAgent method repeatedly calls
this.executorRegistry.list() and builds an array per outcome; change it to
maintain a cached Set<string> of known agent names (e.g., this._knownAgents) and
have _isRegisteredAgent check that Set (fall back to calling list() on first
access). Update the cache whenever the registry changes by listening for
registry change events or by adding a small helper method (e.g.,
refreshKnownAgents) that calls this.executorRegistry.list(), rebuilds the Set,
and is invoked on registry update points; ensure any null/undefined checks for
this.executorRegistry remain and the cache is invalidated or refreshed when
executors are added/removed.

In `@src/pr-remediator.test.ts`:
- Around line 1007-1142: The test suite lacks an integration-level test that
exercises the real production path so hitlPolicy isn't dropped between
world.state.updated → _selfDispatchRemediation() → _handleMergeReady(); add a
new test that installs PrRemediatorPlugin on an InMemoryEventBus, captures
"hitl.request.pr.merge.#", emits the real merge-ready trigger (the same message
format used by world.state.updated/pr.remediate.merge_ready) with
payload.meta.hitlPolicy set (e.g., { ttlMs: 1_800_000, onTimeout: "approve" }),
waits/flushes microtasks, and asserts the captured HITLRequest contains the same
ttlMs and onTimeout values to ensure _extractHitlPolicy and the production flow
(_selfDispatchRemediation, _handleMergeReady, _emitHitlApproval) propagate the
policy end-to-end.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 12091d6b-7198-4c3c-8cf1-0cc8e010a7b8

📥 Commits

Reviewing files that changed from the base of the PR and between 14e2045 and 4001cf7.

📒 Files selected for processing (48)

• .claude/commands/rollcall.md
• .env.dist
• README.md
• __tests__/goal_evaluator_plugin.test.ts
• docs/reference/ceremony-plugin.md
• docs/reference/env-vars.md
• docs/reference/http-api.md
• lib/plugins/pr-remediator.ts
• src/api/__tests__/a2a-server.test.ts
• src/api/__tests__/agent-card.test.ts
• src/api/a2a-server.ts
• src/api/agent-card.ts
• src/api/github.ts
• src/config/env.ts
• src/index.ts
• src/loaders/ceremonyYamlLoader.ts
• src/planner/__tests__/end-to-end-loop.test.ts
• src/planner/__tests__/validate-action-executors.test.ts
• src/planner/types/action.ts
• src/planner/validate-action-executors.ts
• src/plugins/CeremonyPlugin.ts
• src/plugins/__tests__/CeremonyPlugin.test.ts
• src/plugins/__tests__/alert-skill-executor-plugin.test.ts
• src/plugins/__tests__/ceremony-skill-executor-plugin.test.ts
• src/plugins/__tests__/pr-remediator-skill-executor-plugin.test.ts
• src/plugins/action-dispatcher-plugin.test.ts
• src/plugins/action-dispatcher-plugin.ts
• src/plugins/agent-fleet-health-plugin.test.ts
• src/plugins/agent-fleet-health-plugin.ts
• src/plugins/alert-skill-executor-plugin.ts
• src/plugins/ceremony-skill-executor-plugin.ts
• src/plugins/goal_evaluator_plugin.ts
• src/plugins/pr-remediator-skill-executor-plugin.ts
• src/pr-remediator.test.ts
• src/schemas/yaml-schemas.ts
• src/telemetry/telemetry-service.ts
• src/world/extensions/CeremonyStateExtension.ts
• src/world/extensions/__tests__/CeremonyStateExtension.test.ts
• workspace/actions.yaml
• workspace/agents.yaml
• workspace/agents/ava.yaml
• workspace/ceremonies/agent-health.yaml
• workspace/ceremonies/board.cleanup.yaml
• workspace/ceremonies/board.health.yaml
• workspace/ceremonies/board.retro.yaml
• workspace/ceremonies/daily-standup.yaml
• workspace/ceremonies/health-check.yaml
• workspace/ceremonies/service-health.yaml

[coderabbitai]

⚠️ Potential issue | 🟠 Major

hitlPolicy is skipped on the path this plugin uses by default.

_extractHitlPolicy(msg) only works for a pr.remediate.merge_ready payload, but the normal autonomous path calls _handleMergeReady() from world.state.updated via _selfDispatchRemediation(). That message has no payload.meta.hitlPolicy, so the new forwarding never reaches _emitHitlApproval() unless some external publisher triggers pr.remediate.merge_ready directly.

Also applies to: 969-969

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

node - <<'NODE'
for (const ttl of [1e20, Number.MAX_VALUE]) {
  try {
    console.log("ttl=", ttl, "expiresAt=", new Date(Date.now() + ttl).toISOString());
  } catch (err) {
    console.error("ttl=", ttl, "->", String(err));
  }
}
NODE

rg -n "hitlPolicy|ttlMs" workspace src lib

Repository: protoLabsAI/protoWorkstacean

Length of output: 6609

---

🏁 Script executed:

sed -n '1220,1280p' lib/plugins/pr-remediator.ts | cat -n

Repository: protoLabsAI/protoWorkstacean

Length of output: 3373

---

🏁 Script executed:

sed -n '1091,1145p' src/pr-remediator.test.ts | cat -n

Repository: protoLabsAI/protoWorkstacean

Length of output: 2047

---

Cap ttlMs to a safe maximum to prevent RangeError in Date constructor.

Number.isFinite() accepts very large finite values (e.g., 1e20) that overflow JavaScript's maximum safe timestamp (~8.64e15). This causes new Date(Date.now() + ttlMs).toISOString() to throw RangeError, breaking the merge-failure fallback. Add validation to enforce ttlMs <= MAX_DATE_MS - Date.now() at both validation (line 1235) and consumption (line 1256) points.

Suggested fix

+const MAX_DATE_MS = 8.64e15;
+
 private _extractHitlPolicy(msg: BusMessage): { ttlMs?: number; onTimeout?: "approve" | "reject" | "escalate" } | undefined {
   const payload = msg.payload as { meta?: Record<string, unknown> } | undefined;
   const meta = payload?.meta;
   if (!meta || typeof meta !== "object") return undefined;
   const policy = (meta as Record<string, unknown>).hitlPolicy;
   if (!policy || typeof policy !== "object") return undefined;
   const p = policy as Record<string, unknown>;
   const out: { ttlMs?: number; onTimeout?: "approve" | "reject" | "escalate" } = {};
-  if (typeof p.ttlMs === "number" && Number.isFinite(p.ttlMs) && p.ttlMs > 0) out.ttlMs = p.ttlMs;
+  if (typeof p.ttlMs === "number" && Number.isFinite(p.ttlMs) && p.ttlMs > 0) {
+    out.ttlMs = Math.min(p.ttlMs, Math.max(1, MAX_DATE_MS - Date.now()));
+  }
   if (p.onTimeout === "approve" || p.onTimeout === "reject" || p.onTimeout === "escalate") {
     out.onTimeout = p.onTimeout;
   }
   return Object.keys(out).length > 0 ? out : undefined;
 }
@@
-  const ttlMs = hitlPolicy?.ttlMs ?? 30 * 60 * 1000;
+  const ttlMs = Math.min(hitlPolicy?.ttlMs ?? 30 * 60 * 1000, Math.max(1, MAX_DATE_MS - Date.now()));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/plugins/pr-remediator.ts` around lines 1227 - 1239, The ttlMs extracted
in _extractHitlPolicy can be an extremely large finite number and later cause
new Date(Date.now() + ttlMs) to throw RangeError; fix by defining a MAX_DATE_MS
constant (e.g. the max safe epoch in ms) and cap ttlMs during extraction in
_extractHitlPolicy (set out.ttlMs = Math.min(p.ttlMs, MAX_DATE_MS - Date.now())
while ensuring non-negative), and also re-validate/cap ttlMs at the consumption
point before calling new Date(...) to guard again against overflow; reference
the _extractHitlPolicy method and the place where ttlMs is used to compute the
deadline so both locations apply the same MAX_DATE_MS cap.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

The HTML-error heuristic is too broad, and the sanitized reply still leaks body text.

Any successful reply that legitimately starts with <!DOCTYPE/<html> or discusses 404 not found will be converted into a failed task here. On top of that, sanitizeHtmlError() echoes the first 200 chars of the upstream page back to the caller, which weakens the stated sanitization goal. Tighten detection to actual HTTP-error signatures and return a fully generic client-facing message while keeping the raw payload only in logs.

Possible fix

 function looksLikeHtmlError(text: string): boolean {
   if (!text) return false;
   const head = text.slice(0, 256).toLowerCase();
-  return (
-    head.startsWith("<!doctype")
-    || head.startsWith("<html")
-    || head.includes("cannot post /")
-    || head.includes("cannot get /")
-    || head.includes("404 not found")
-  );
+  const looksLikeMarkup = head.startsWith("<!doctype") || head.startsWith("<html");
+  const hasHttpErrorSignature =
+    head.includes("cannot post /")
+    || head.includes("cannot get /")
+    || head.includes("404 not found")
+    || head.includes("500 internal server error");
+  return looksLikeMarkup && hasHttpErrorSignature;
 }
 
-function sanitizeHtmlError(raw: string): string {
-  const hint = raw
-    .replace(/<!DOCTYPE[^>]*>/gi, "")
-    .replace(/<[^>]+>/g, " ")
-    .replace(/\s+/g, " ")
-    .trim()
-    .slice(0, 200);
-  return (
-    "Downstream agent returned an HTTP error (possibly a card misconfiguration). "
-    + "See workstacean logs for details. "
-    + `[debug hint: ${hint}]`
-  );
+function sanitizeHtmlError(_raw: string): string {
+  return "Downstream agent returned an HTTP error. See workstacean logs for details.";
 }

Also applies to: 163-175

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/api/a2a-server.ts` around lines 49 - 74, The current looksLikeHtmlError
and sanitizeHtmlError are too permissive and leak body content; change
looksLikeHtmlError to detect explicit HTTP-error signatures only
(case-insensitive exact phrases like "404 Not Found", "500 Internal Server
Error", common server banners such as "nginx/", "apache/", "cloudflare", and
regexes matching "<title>\\d{3}" or "cannot POST /" with word boundaries) rather
than any occurrence of "404 not found" or starting with "<!doctype"/"<html>".
Update sanitizeHtmlError to never return upstream body text to clients — return
a fully generic message like "Downstream agent returned an HTTP error; see
server logs for details" and write the full raw payload to the server log (use
the existing logger used elsewhere in this file) for debugging. Apply the same
tightened logic where these helpers are reused (the other occurrence referenced
in the review).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Only default to Ava when there is no routing hint.

This now injects targets: ["ava"] for every target-less request, including calls that already supplied a skillHint. Since the shared agent card advertises fleet-wide skills, that hard-pin bypasses normal skill-based routing and will fail non-Ava skills like protomaker/quinn handlers. Leave targets unset when a skill hint is present, and only apply the Ava default when neither target nor routing hint was provided.

Possible fix

-    let targets: string[];
-    if (explicitTargets.length === 0) {
-      targets = ["ava"];
-      console.log("[a2a-server] no target specified, defaulting to [ava] for message/send");
-    } else {
-      targets = explicitTargets;
-    }
+    const hasExplicitSkillHint =
+      (typeof metadata.skillHint === "string" && metadata.skillHint.length > 0)
+      || (typeof metadata.skill === "string" && metadata.skill.length > 0);
+    const targets =
+      explicitTargets.length > 0
+        ? explicitTargets
+        : hasExplicitSkillHint
+          ? undefined
+          : ["ava"];
+    if (!targets) {
+      // let SkillDispatcherPlugin resolve by skill
+    } else if (explicitTargets.length === 0) {
+      console.log("[a2a-server] no target specified, defaulting to [ava] for message/send");
+    }
...
-        targets,
+        ...(targets ? { targets } : {}),

Also applies to: 243-246

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/api/a2a-server.ts` around lines 114 - 127, The current logic
unconditionally sets targets = ["ava"] when metadata.targets is empty, which
overrides routing based on a provided skillHint; change the branch in the
handler (the explicitTargets / targets logic) to only set targets = ["ava"] when
there are no explicitTargets AND metadata.skillHint (or equivalent routing hint
field) is falsy — if a skillHint exists leave targets undefined so skill-based
routing can proceed; apply the same fix to the other occurrence around the
similar targets defaulting (lines ~243-246) to ensure we don't hard-pin Ava when
a routing hint is present.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Don't mock the bus in this test.

validateActionExecutors() publishes a normal bus message, so this coverage should exercise the real in-memory bus path instead of a mock() wrapper. That keeps the test aligned with the rest of the suite and avoids validating a fake transport.

As per coding guidelines, **/*.test.{ts,tsx}: Write unit tests with bun test against in-memory bus with no mocks and no LLM calls.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/planner/__tests__/validate-action-executors.test.ts` around lines 148 -
156, The test is mocking the bus instead of using the real in-memory bus, so
replace the fake bus object used in the validateActionExecutors(actions,
executors, { bus: bus as never }) call with the actual in-memory bus instance
used by your codebase (the same Bus implementation exercised by other tests) so
publish/subscribe go through the real transport; locate the mocked symbols
(published array, bus.subscribe, bus.unsubscribe, bus.publish, BusMessage) and
remove the mock(...) wrappers, instantiate the real in-memory bus, pass it into
validateActionExecutors, and assert published messages via the real bus API to
keep the test aligned with other *.test.ts files and avoid mocking the
transport.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Use the shared in-memory bus in this test file.

This stub is close enough to satisfy the current assertions, but it is still a separate event-bus implementation with its own behavior. That weakens the value of these tests and conflicts with the repo’s test rule.

As per coding guidelines, **/*.test.{ts,tsx}: Write unit tests with bun test against in-memory bus with no mocks and no LLM calls.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugins/__tests__/alert-skill-executor-plugin.test.ts` around lines 11 -
29, The test defines a local makeBus stub
(functions/published/subscribe/publish/topics/unsubscribe) which duplicates
event-bus behavior; replace this local makeBus with the project's shared
in-memory bus used by other tests (import the shared in-memory bus factory or
singleton and use it instead of makeBus) so the test uses the canonical bus
implementation and complies with the no-mocks in-memory-bus rule; update
references to published, subscribe, publish, topics and unsubscribe to use the
imported bus API.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Use InMemoryEventBus for these tests instead of a local stub.

A hand-rolled bus makes this suite less representative than the rest of the plugin tests and can hide contract drift in publish/subscribe behavior.

As per coding guidelines, **/*.test.{ts,tsx}: Write unit tests with bun test against in-memory bus with no mocks and no LLM calls.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugins/__tests__/ceremony-skill-executor-plugin.test.ts` around lines 11
- 29, Replace the hand-rolled makeBus stub with the shared InMemoryEventBus used
across plugin tests: import InMemoryEventBus and instantiate it in place of
makeBus(), then update references that relied on the local published array or
custom methods to use InMemoryEventBus's publish/subscribe/unsubscribe and any
provided inspection helpers (e.g., getPublished or events history) so tests
exercise the real in-memory publish/subscribe contract; remove the makeBus
function and adapt assertions to the InMemoryEventBus API in the
ceremony-skill-executor-plugin test.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Use InMemoryEventBus here instead of a bespoke stub.

This helper skips the real bus semantics the production plugins rely on, so these unit tests can stay green while the actual event-bus contract drifts. Replacing makeBus() with the shared in-memory bus would make the coverage match the rest of the suite.

As per coding guidelines, **/*.test.{ts,tsx}: Write unit tests with bun test against in-memory bus with no mocks and no LLM calls.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugins/__tests__/pr-remediator-skill-executor-plugin.test.ts` around
lines 13 - 30, Replace the bespoke makeBus() test stub with the shared
InMemoryEventBus: import or require InMemoryEventBus in the test file and
instantiate it instead of calling makeBus(), then use the InMemoryEventBus
instance's subscribe, unsubscribe, publish and topics APIs (and its typed
BusMessage usage) so tests exercise the real in-memory semantics; remove the
makeBus() function and update any references in
pr-remediator-skill-executor-plugin.test.ts to call the InMemoryEventBus
instance methods (e.g., subscribe/unsubscribe/publish/topics) so the test uses
the canonical event-bus behavior.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

These “integration” tests never prove the remediator handled the message.

Both cases still pass if execute() publishes to a dead topic, because the only extra assertion is that a subscription exists on the bus. Add an observable assertion after dispatch — e.g. subscribe to the remediator topic or assert a remediator side effect — so the test actually covers the executor → bus → handler path it claims to verify.

Also applies to: 206-228

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugins/__tests__/pr-remediator-skill-executor-plugin.test.ts` around
lines 179 - 204, The test currently only checks that the topic
"pr.remediate.update_branch" exists and that
ExecutorRegistry.resolve("action.pr_update_branch") returns an executor, but it
never verifies that PrRemediatorPlugin actually received the dispatched message;
update the test to observe the remediator handling by subscribing or spying on
the bus for the remediator topic before calling executor.execute (e.g., use
InMemoryEventBus.subscribe("pr.remediate.update_branch", ...) or capture
bus.publish calls) and assert that the subscriber was invoked or that an
expected remediator side-effect occurred after executor.execute completes;
reference the existing InMemoryEventBus, PrRemediatorPlugin.install/uninstall,
ExecutorRegistry.resolve("action.pr_update_branch"), and executor.execute call
to locate where to add the observable assertion.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don't start the cooldown window before the action is actually admitted.

Line 174 records lastDispatchedAt before the target guard and before tryDispatch(). That means a dispatch dropped as target_unresolved — or one left pending because WIP is full — still consumes the cooldown window, so fixing the registry or freeing capacity does not allow an immediate retry. The timestamp should only be written once the action has actually been accepted for execution.

Also applies to: 231-245

[coderabbitai]

🧹 Nitpick comments (1)

src/pr-remediator.test.ts (1)

983-1143: Remove duplicated hitlPolicy suite to avoid double-running the same tests.

The block starting at Line 993 duplicates the existing describe("PrRemediatorPlugin — hitlPolicy") suite already present later (Line 1155+). Keep a single suite (preferably this expanded one) and delete the duplicate block to prevent redundant CI work and split maintenance.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/pr-remediator.test.ts` around lines 983 - 1143, Delete the duplicated
test suite describe("PrRemediatorPlugin — hitlPolicy") (the earlier block
containing tests referencing privateOf(plugin)._emitHitlApproval and
_extractHitlPolicy) so only one copy remains; keep the expanded/desired version
and remove the other duplicate block that runs the same tests to avoid double
execution in CI.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/pr-remediator.test.ts`:
- Around line 983-1143: Delete the duplicated test suite
describe("PrRemediatorPlugin — hitlPolicy") (the earlier block containing tests
referencing privateOf(plugin)._emitHitlApproval and _extractHitlPolicy) so only
one copy remains; keep the expanded/desired version and remove the other
duplicate block that runs the same tests to avoid double execution in CI.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ea2eb94e-36b6-47d7-97a9-bd0c47490e9a

📥 Commits

Reviewing files that changed from the base of the PR and between 4001cf7 and 705f2b5.

📒 Files selected for processing (1)

• src/pr-remediator.test.ts