Conversation
- unify extraction and synthesis provider runtime plumbing - route interactive paths through shared provider access - stabilize workspace and native test execution under bun Assisted-by: Forge
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- restore legacy OpenAI behavior for OS chat and agent when configured - share exported FTS query builder instead of duplicating it - tighten daemon startup helper naming and synthesis startup options - preserve backoff recovery expectations in worker tests Assisted-by: Forge
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- preserve degraded synthesis startup instead of crashing daemon boot - harden OpenAI fallback cache/error handling for interactive routes - restore vector-runtime guard and dead-job backoff expectations Assisted-by: Forge
PR-Reviewer-Ant
left a comment
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Automated review by pr-reviewer | model: claude-sonnet-4-6 | commit:
64a88be6
Harness output could not be parsed as structured JSON.
I'll base the review on the diff and changed file contents provided in the prompt. Let me analyze thoroughly.
{
"summary": "[Automated Review — pr-reviewer bot] This PR consolidates LLM provider plumbing across extraction, synthesis, and widget roles into a shared registry in `llm.ts`, moves provider-resolution logic into a dedicated module, hardens FTS query building, and stabilises Bun workspace source resolution. Several issues were found: one blocking behavioral regression in OpenAI API key caching, one warning-level duplication divergence, and one warning-level behavior change in the worker retry gate that is technically not pure refactor.",
"verdict": "request_changes",
"confidence": {
"level": "high",
"reasons": ["sufficient_diff_evidence", "targeted_context_included"],
"justification": "The key caching regression is fully provable from the diff: the old per-route `let cachedApiKey: string | null = null` pattern re-fetched on every call when the key was absent (`null` and `\"\"` are both falsy), while the new `cachedOpenAiApiKey !== undefined` guard in `llm.ts:83-87` permanently caches a missing key as `\"\"` after the first failed lookup. The duplication in `daemon.ts` is visible by comparing the inline synthesis-effective logic at line ~12086–12094 against the exported `resolveEffectiveRuntimeProvider` defined in `provider-resolution.ts`. The worker retry gate change is evident from the `failJob` signature change and the `effectiveMaxAttempts` clamping at `worker.ts:1560`."
},
"ui_screenshot_needed": false,
"comments": [
{
"file": "packages/daemon/src/llm.ts",
"line": 83,
"body": "**Blocking – OpenAI API key cache permanently poisons on first missing-key lookup.**\n\nPrevious per-route code (`os-chat.ts`, `os-agent.ts`) used `let cachedApiKey: string | null = null` with `if (!cachedApiKey)` as the guard. Because both `null` and `\"\"` are falsy, every call re-attempted the secret lookup when the key was absent, allowing dynamically-added secrets to be picked up without a restart.\n\nThe new `getOpenAiApiKey()` uses `if (cachedOpenAiApiKey !== undefined)` as its gate. On a missing key, `cachedOpenAiApiKey` is set to `\"\"` (line ~87: `process.env.OPENAI_API_KEY || (await getSecret(...).catch(() => \"\"))`). `\"\" !== undefined` is true, so all subsequent calls immediately throw `MissingOpenAiApiKeyError` without re-querying the environment or secrets store. The cache is only reset via `closeProvider()`, which only fires when both providers are closed — not a normal runtime event.\n\nConsequence: a user who starts the daemon before setting `OPENAI_API_KEY` and then adds it via `signet secrets set OPENAI_API_KEY` will receive errors until the daemon is restarted. This regresses the existing live-configuration behavior.\n\nSuggestion: use `null` (not `\"\"`) as the \"definitely not found\" sentinel and retain a retry/TTL path, or at minimum document the restart requirement.",
"severity": "blocking"
},
{
"file": "packages/daemon/src/daemon.ts",
"line": 12086,
"body": "**Warning – inline synthesis effectiveProvider logic duplicates the exported `resolveEffectiveRuntimeProvider`.**\n\n`resolveEffectiveRuntimeProvider` is defined and exported from `provider-resolution.ts` (handles both extraction and synthesis cases). In `resolveAndLogRuntimeStartup`, instead of calling that function for the synthesis branch, the same ternary chain is re-implemented inline:\n```ts\nconst effectiveProvider: RuntimeSynthesisProviderName | null =\n runtime.status === 'paused'\n ? null\n : cfg.enabled\n ? runtime.effectiveProvider === 'command' ? 'none' : runtime.effectiveProvider\n : null;\n```\nIf the logic in `resolveEffectiveRuntimeProvider` is ever updated, this inline copy will silently diverge. AGENTS.md §5 ("Duplication / parity drift") explicitly flags this pattern. The fix is to call `resolveEffectiveRuntimeProvider(cfg, runtime)` here.",
"severity": "warning"
},
{
"file": "packages/daemon/src/pipeline/worker.ts",
"line": 1560,
"body": "**Warning – `effectiveMaxAttempts` clamping is a behavior change disguised as refactor.**\n\nThe PR is typed as `refactor`. However, the new code adds:\n```ts\nconst effectiveMaxAttempts = Math.max(1, Math.min(job.max_attempts, pipelineCfg.worker.maxRetries));\nfailJob(db, job.id, msg, job.attempts, job.max_attempts, effectiveMaxAttempts);\nif (job.attempts >= effectiveMaxAttempts) { ... }\n```\nPreviously, `job.max_attempts` was the sole threshold for transitioning a job to `dead`. Now, if `pipelineCfg.worker.maxRetries < job.max_attempts`, a job will be marked dead earlier than its stored `max_attempts` would indicate. Jobs can now be killed by a config change that reduces `maxRetries` without migrating stored `max_attempts` values. The PR checklist acknowledges that `Input/config validation and bounds checks added` was not completed, which is relevant here. The associated tests were adjusted to match (e.g., `maxRetries: 1 → 2`, extended sleep) rather than asserting the old contract was preserved.",
"severity": "warning"
},
{
"file": "packages/core/package.json",
"line": 11,
"body": "**Nitpick – `\"bun\": \"./src/index.ts\"` exposes raw TypeScript source as the published package entry point under Bun.**\n\nAdding `src` to `files` and a `bun` export condition means downstream consumers running under Bun receive the TypeScript sources directly rather than the compiled `dist/`. This can cause unexpected behavior if those consumers have different `tsconfig` settings or if `src/` transitively imports devDependencies. Intentional for workspace-internal test resolution, but the blanket `\"bun\"` condition in the exported `package.json` affects external consumers too. Consider scoping this to workspace-internal usage (e.g., via a separate internal entry or Bun-specific workspace override) rather than publishing it.",
"severity": "nitpick"
}
]
}
PR-Reviewer-Ant
left a comment
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Automated review by pr-reviewer | model: claude-sonnet-4-6 | commit:
5d84c8fe
[Automated Review — pr-reviewer bot] PR #462 introduces a new routing core module and CLI surface. Found one blocking correctness bug in the scoring engine, one warning-level type-safety gap in the legacy compiler, and two process compliance gaps (typecheck/lint failing) that the author acknowledges but leaves open.
Confidence: High [sufficient_diff_evidence, targeted_context_included] - The score inversion in buildCandidateTrace is directly visible in the routing.ts diff — Array.prototype.indexOf returns -1 for absent elements, making 100 - (-1 * 5) = 105, which outscores any explicitly preferred target. The executor type assignment in compileLegacyRoutingConfig is also directly visible in the diff and unambiguously passes PipelineProviderChoice values as RoutingExecutorKind without narrowing. The typecheck/lint failures are author-disclosed in the PR notes. No runtime trace is needed to confirm the first two — the arithmetic and the unguarded assignment are provable from the diff alone.
| const expectedInputTokens = | ||
| request.expectedInputTokens ?? config.taskClasses[classification.taskClass]?.expectedInputTokens; | ||
| if (expectedInputTokens && model.contextWindow && expectedInputTokens > model.contextWindow) { | ||
| blockedBy.push(`context window too small (${model.contextWindow})`); |
There was a problem hiding this comment.
Score inversion bug — blocking correctness. orderedTargets.indexOf(targetRef) returns -1 when the candidate is not in the preferred list (e.g. a roster-only candidate). That makes 100 - (-1 * 5) = 105, which exceeds the score of the top-ranked preferred target (100). In automatic mode, any candidate that is NOT explicitly preferred will outscore every explicitly preferred one — inverting the entire preference ordering.
Fix: guard the expression or substitute a penalizing value when idx < 0:
const idx = orderedTargets.indexOf(targetRef);
if (idx >= 0) {
score += 100 - idx * 5;
reasons.push(`preferred order ${idx + 1}`);
} else {
score += 0; // no preference bonus
}The reasons.push check on the next line already has the correct includes guard but the score has already been wrongly bumped before it.
| const routingRaw = embeddedRouting ?? standaloneRouting; | ||
| if (!routingRaw) { | ||
| return ok(base); | ||
| } |
There was a problem hiding this comment.
Executor type gap in legacy compiler. compileLegacyRoutingConfig assigns executor: opts.extraction.provider and executor: opts.synthesis.provider directly into RoutingTargetConfig.executor (typed as RoutingExecutorKind). PipelineProviderChoice includes values like "openai" that are absent from ROUTING_EXECUTOR_KINDS (["claude-code","codex","opencode","anthropic","openrouter","ollama","openai-compatible"]). Any legacy install using provider: openai would store an invalid executor string in the compiled routing config, causing parseTargetConfig to silently drop that target if it is later round-tripped through the parser (which guards with ROUTING_EXECUTOR_KINDS.includes), and silently routing to no extraction target. This is likely contributing to the acknowledged typecheck failures. A narrowing step or explicit provider-to-executor mapping is needed here.
| maxTokens, | ||
| refresh: options.refresh === true, | ||
| }); | ||
| if (!ok) { |
There was a problem hiding this comment.
signet route pin <targetRef> writes the value to agent.yaml without validating the target/model format. parseRoutingTargetRef is already exported from @signet/core — call it here and print an error before writing if the ref is malformed. A typo like opus46 (missing slash) would be silently pinned and then permanently fail routing until manually corrected.
| attempts?: Array<{ targetRef: string; ok: boolean; error?: string }>; | ||
| }; | ||
| console.log(chalk.bold("\n Route test\n")); | ||
| console.log(chalk.dim(` Selected: ${result.decision?.targetRef ?? "-"}`)); |
There was a problem hiding this comment.
writeAgentYaml and the callers (pin, unpin) have no error handling. If stringifyYamlDocument or writeFileSync throws, the process exits with an unhandled exception rather than a user-readable message. Wrap in try/catch and print a friendly error before process.exit(1).
| } | ||
|
|
||
| const traces = candidateRefs.map((targetRef) => | ||
| buildCandidateTrace( |
There was a problem hiding this comment.
Redundant double-block for local_only privacy. When requiredPrivacy === "local_only" and the target is not local, both guards fire: privacyRank(targetPrivacy) < privacyRank("local_only") is true (first push), then requiredPrivacy === "local_only" && target.kind !== "local" is also true (second push). The trace.candidates[].blockedBy array for that candidate will contain two entries for one logical reason. explain and doctor consumers will display duplicated blocking messages. The second check is redundant and can be removed.
| .command("status") | ||
| .description("Show routing health and workload bindings") | ||
| .action(async (options) => { | ||
| const status = await deps.fetchFromDaemon<RouteStatusResponse>("/api/inference/status"); |
There was a problem hiding this comment.
signet route list and signet route status have identical action implementations — both call /api/inference/status and both call printStatus. The docs describe them as distinct (list = config + health, status = health + workload bindings). Either differentiate the output or consolidate to one command. As-is, status is a no-op alias with a misleading description.
| candidates: traces, | ||
| }; | ||
|
|
||
| const allowed = traces.filter((candidate) => candidate.allowed); |
There was a problem hiding this comment.
classifyRouteRequest(config, request) is called twice in resolveRoutingDecision — once as an argument to orderedPreferenceLists, and again on the next line to assign classification. The first call's result is available as the third parameter passed to orderedPreferenceLists but discarded. Extract the result to a const before the orderedPreferenceLists call and reuse it.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
PR-Reviewer-Ant
left a comment
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Automated review by pr-reviewer | model: claude-sonnet-4-6 | commit:
5e603758
[Automated Review — pr-reviewer bot] PR #462 introduces a new inference routing layer, new API endpoints, and signet route CLI commands. The implementation is substantial and mostly well-structured, but several correctness, convention, and security-documentation issues are flagged below. The PR is labeled refactor but ships user-visible CLI surface that requires feat: per repo convention, typecheck is admittedly broken across several packages, and two distinct localhost-bypass security descriptions now contradict each other across docs.
Confidence: Medium [sufficient_diff_evidence, missing_cross_module_context] - The routing.ts decision engine diff is truncated at ~750 lines, so the full candidate-scoring and fallback logic cannot be audited. The daemon source for the auth-middleware localhost bypass is not in the diff at all, so whether the TCP-vs-Host change in SELF-HOSTING.md reflects an actual implementation change or just a doc correction cannot be confirmed from this diff alone. The CLI and core routing changes are fully visible and the findings below are grounded in those concrete changed lines.
Unmapped findings (not on changed lines):
- docs/AUTH.md:35 - AUTH.md still contains: 'localhost detection in hybrid mode works by checking the
Hostrequest header, not the TCP peer address.' This PR updated SELF-HOSTING.md (line ~288) to say the bypass now checks the actual socket peer address, but AUTH.md was not updated. The two docs now give directly contradictory security descriptions. Operators making deployment decisions based on AUTH.md will have incorrect expectations about what is and is not spoofable in hybrid mode. AUTH.md must be updated to match SELF-HOSTING.md (or vice-versa if the daemon was not actually changed).
| @@ -0,0 +1,377 @@ | |||
| import { existsSync, readFileSync, writeFileSync } from "node:fs"; | |||
There was a problem hiding this comment.
The signet route subcommand group is a new user-facing CLI surface (list, status, doctor, explain, test, pin, unpin). Per AGENTS.md conventions, feat: is reserved for user-facing features and bumps the minor version. This PR is labeled refactor, which means no minor-version bump will be triggered. If semver is enforced automatically from commit type, downstream consumers will not see the expected version signal for a new command surface.
| routeCmd | ||
| .command("test <prompt>") | ||
| .description("Execute a real routed prompt") | ||
| .option("--agent <agent>", "Agent id") |
There was a problem hiding this comment.
Number.parseInt(options.maxTokens, 10) returns NaN when --max-tokens receives a non-numeric string (e.g. signet route test ... --max-tokens abc). The falsy-check on options.maxTokens only guards for an absent flag, not for non-numeric input. NaN is then sent to the API as maxTokens. The API spec says this field is clamped, but the behavior of clamping NaN is undefined. Add a guard: const parsed = Number.parseInt(options.maxTokens, 10); if (!Number.isFinite(parsed) || parsed <= 0) { console.error(...); process.exit(1); }
| ): RouterResult<never> { | ||
| return { ok: false, error: { code, message, ...(details ? { details } : {}) } }; | ||
| } | ||
|
|
There was a problem hiding this comment.
hasStandaloneRoutingShape matches if the raw object contains any of "enabled", "targets", "agents", "policies", "workloads", etc. A standard agent.yaml already uses "enabled" as a top-level flag for several features (memory.pipelineV2.enabled, update flags, etc.). If parseRoutingConfig is ever called with an already-flattened config fragment that carries an "enabled" key at root, the entire fragment is mistakenly treated as a standalone routing block, silently overwriting the legacy base config instead of merging it. The heuristic should require at least two signals ("targets" + one other) or be scoped to explicit call sites that already know they are receiving a standalone routing block.
| } | ||
|
|
||
| function isRecord(value: unknown): value is Record<string, unknown> { | ||
| return typeof value === "object" && value !== null && !Array.isArray(value); |
There was a problem hiding this comment.
writeAgentYaml serializes the parsed JS object back to YAML via stringifyYamlDocument. Round-tripping through parse+stringify will silently strip all comments and likely alter hand-crafted formatting in agent.yaml. For route pin / route unpin this means a user who has extensively commented their agent.yaml loses all those annotations on the first pin operation. Consider a targeted YAML patch strategy (e.g. modify only the routing.agents.<id>.pinnedTargets path) rather than a full reserialize.
| | inferenceExplain | 120 | | ||
| | inferenceExecute | 20 | | ||
| | inferenceGateway | 30 | | ||
| | recallLlm | 60 | |
There was a problem hiding this comment.
The x-signet-actor header has been removed from the docs and the rate-limiting description changed so all unauthenticated requests now share the "anonymous" bucket. This is a behavioral change (previously per-actor buckets could be attributed in hybrid mode via the header). No daemon source diff is present to confirm whether the implementation was actually changed or only the docs were updated. If the daemon still accepts the header, the docs are wrong. If the daemon was changed, this is a breaking change that was not listed as such and has no migration note. Clarify which case this is.
| @@ -0,0 +1,1129 @@ | |||
| import type { PipelineExtractionConfig, PipelineSynthesisConfig } from "./types"; | |||
There was a problem hiding this comment.
The diff is truncated and resolveRoutingDecision — specifically the candidate-scoring loop, fallback-chain construction, and strict-mode handling — cannot be fully audited. The test in routing.test.ts covers the local-only privacy gate and explicit-target-outside-roster rejection, which is good, but the strict and hybrid policy modes are not covered by any visible test. Given that privacy-tier enforcement is a hard security gate per the spec, the missing coverage for strict mode (which uses an explicit ordered chain) is a gap.
| .action(async (options) => { | ||
| const status = await deps.fetchFromDaemon<RouteStatusResponse>("/api/inference/status"); | ||
| if (!status) { | ||
| console.error(chalk.red("Failed to get routing status from daemon")); |
There was a problem hiding this comment.
route status and route list have identical implementations: both call GET /api/inference/status and both call printStatus. There is no behavioral difference. Either document the intended distinction or collapse them into one command with an alias.
| @@ -667,6 +885,15 @@ auth: | |||
| modify: | |||
There was a problem hiding this comment.
recallLlm appears in the rate-limit reference table (line ~920) but is absent from the YAML config example immediately above it. The same gap exists in docs/AUTH.md. Operators copying the YAML snippet will not know how to override recallLlm. Add it to both YAML examples for consistency.
| closed = true; | ||
| controller.close(); | ||
| }; | ||
| const safeError = (error: Error) => { |
PR-Reviewer-Ant
left a comment
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Automated review by pr-reviewer | model: claude-sonnet-4-6 | commit:
2ef7c5e2
Automated bot review (pr-reviewer). PR #462 introduces a large new routing module, new inference API endpoints, CLI route commands, and test/workspace stabilization. Several issues warrant attention before merge: a mandatory typecheck gate is explicitly failing, the hybrid-mode security fix is documented but the implementation is absent from the visible diff, a silent breaking change removes the x-signet-actor rate-limit attribution header, and src/ is unexpectedly added to published npm artifacts.
Confidence: Medium [missing_cross_module_context, sufficient_diff_evidence] - The core routing decision algorithm (resolveRoutingDecision) and the fallback/scoring logic dominate the spec contract but fall in the truncated tail of packages/core/src/routing.ts. The Hono auth middleware that implements the documented hybrid-mode TCP-peer-address change is also absent from the diff, making it impossible to verify the security fix exists in code. Issues 1–6 below are directly provable from the visible diff; issue 4 requires seeing the middleware to confirm.
| @@ -0,0 +1,1129 @@ | |||
| import type { PipelineExtractionConfig, PipelineSynthesisConfig } from "./types"; | |||
There was a problem hiding this comment.
Blocking: bun run typecheck is explicitly failing and the PR checklist box is unchecked. AGENTS.md lists 'Lint/typecheck/tests pass locally' as a mandatory pre-review requirement. The author notes the failures are caused by cross-package rootDir boundary violations in @signet/connector-*, @signet/opencode-plugin, and @signet/oh-my-pi-extension. These must be resolved or the failures must be explicitly scoped out with a documented exemption before merge, because type errors introduced by this refactor may silently break the routing plumbing in those packages.
|
|
||
| This is a practical default for development machines where you want the | ||
| local dashboard and CLI to work without tokens, but also want to call the | ||
| daemon from CI pipelines or remote agents. Note the localhost bypass is |
There was a problem hiding this comment.
Warning: The docs now claim hybrid-mode localhost bypass uses the TCP peer address instead of the Host header — a security-meaningful change that fixes spoofability. However, the daemon auth middleware source (likely packages/daemon/src/auth/) is absent from this diff. If the code was not updated, these docs are misleading and the Host-header spoofing vector still exists. If the code was updated, the implementation should be in this PR and reviewable. Please confirm the middleware change is included or identify the commit that contains it.
| `"anonymous"` bucket. | ||
|
|
||
| When a limit is exceeded, the daemon returns `429 Too Many Requests` with | ||
| a `Retry-After` header indicating how many seconds until the window resets. |
There was a problem hiding this comment.
Warning: Removing x-signet-actor header support is a behavioral breaking change. The old behavior let unauthenticated local tools in hybrid mode claim separate rate-limit buckets; the new behavior collapses all unauthenticated requests into one shared "anonymous" bucket. With inferenceExecute limited to 20/min, two concurrent CLI tools hitting the router will exhaust each other's budget. This removal is undocumented in the PR description ('no UI changes') and is not a pure refactor. If intentional, document the migration impact.
| @@ -6,9 +6,6 @@ | |||
| export { Signet } from "./signet"; | |||
There was a problem hiding this comment.
Warning: Agent, AgentManifest, and AgentConfig are moved from value exports (export { ... }) to type-only exports (export type { ... }). If any of these are classes or are used with instanceof checks elsewhere in the monorepo, this is a runtime breaking change that TypeScript may not catch before the typecheck is fixed. Similarly, MigrationSource is moved to export type. Verify that all three former value exports are interfaces/type aliases and not classes.
| ".": { | ||
| "bun": "./src/index.ts", | ||
| "import": "./dist/index.js", | ||
| "types": "./dist/index.d.ts" |
There was a problem hiding this comment.
Warning: Adding "bun": "./src/index.ts" to the exports map required adding "src" to the files array, which means TypeScript source files will be published to npm. The same pattern is in packages/connector-base/package.json and packages/core/package.json. Shipping source alongside dist significantly increases package size and could confuse non-bun Node.js consumers who accidentally resolve to TS sources. Confirm this is intentional and that it does not break the existing connector install path under plain Node.
| function writeAgentYaml(path: string, data: Record<string, unknown>): void { | ||
| writeFileSync(path, stringifyYamlDocument(data)); | ||
| } | ||
|
|
There was a problem hiding this comment.
Warning: writeAgentYaml round-trips agent.yaml through parseYamlDocument → mutate → stringifyYamlDocument. YAML comments, key ordering, and inline formatting in a hand-crafted agent.yaml will be silently destroyed on the first signet route pin or signet route unpin. Users who rely on comments in agent.yaml for documentation or who version-control the file will see noisy diffs. Consider at minimum warning in the command output that formatting will change, or using a smarter in-place patch strategy.
| console.log(JSON.stringify(status, null, 2)); | ||
| return; | ||
| } | ||
| printStatus(status); |
There was a problem hiding this comment.
Nitpick: signet route list and signet route status have identical implementations — both fetch /api/inference/status and call printStatus. This duplicates ~20 lines and diverges from AGENTS.md's 'Duplicate logic is a smell. Extract shared code where it clarifies.' guideline. One should delegate to the other, or both should share a single helper.
| const decision = data as { | ||
| targetRef?: string; | ||
| policyId?: string; | ||
| taskClass?: string; |
There was a problem hiding this comment.
Nitpick: data as { targetRef?: string; policyId?: string; ... } and similar casts in explain and test actions use as to coerce unknown rather than narrowing. AGENTS.md explicitly prohibits as and requires narrowing with explicit null checks. The risk here is low (display-only paths), but it sets a precedent for the new surface.
| // mocking @signet/core globally, which otherwise leaks into later suites. | ||
| const signet = await import("./index"); | ||
| const signetPlugin = signet.default; | ||
| const { memoryStore, _resetRegistration } = signet; |
There was a problem hiding this comment.
Nitpick: The module is imported at top-level (const signet = await import('./index')) before any beforeEach sets process.env.SIGNET_PATH. The original mock intercepted readStaticIdentity to handle the case where ~/.agents does not exist at import time. The new approach relies on SIGNET_PATH being set before the module exercises readStaticIdentity, but SIGNET_PATH is only set inside beforeEach. If the module calls readStaticIdentity during its own initialization phase (the original comment said 'import after mock so the module picks up the stub'), this could produce different behavior on CI hosts where ~/.agents does not exist. The PR notes tests pass locally — verify they also pass on a clean CI image where ~/.agents is absent.
2 participants
Summary
Unify the daemon's LLM provider plumbing around shared runtime roles, route interactive consumers through the same provider access layer, and stabilize Bun workspace test execution for the affected connector/native paths.
Changes
dist/outputs.Type
feat— new user-facing feature (bumps minor)fix— bug fixrefactor— restructure without behavior changechore— build, deps, config, docsperf— performance improvementtest— test coveragePackages affected
@signet/core@signet/daemon@signet/cli/ dashboard@signet/sdk@signet/connector-*@signet/webpredictor@signet/native,packages/adapters/openclawScreenshots
N/A — no UI changes.
PR Readiness (MANDATORY)
INDEX.md+dependencies.yaml)Migration Notes (if applicable)
Testing
bun testpassesbun run typecheckpassesbun run lintpassesAI disclosure
Assisted-bytags in commits)Notes
bun testpasses locally with1531 pass / 37 skip / 0 fail; the skips are limited to native addon parity tests when the platform.nodebinary is absent.bun run typecheckstill fails in multiple workspace packages because source-export/path-based resolution pulls cross-package sources under per-packagerootDirboundaries (for example in@signet/connector-*,@signet/opencode-plugin, and@signet/oh-my-pi-extension).bun run lintstill reports broad pre-existing repository diagnostics; this branch fixes formatting in the touched files it introduced but does not resolve the repo-wide backlog.