feat(gateway): Unit 1 — extract @fro-bot/runtime package#541
Conversation
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: CONDITIONAL
This extraction is architecturally sound and the bulk of it is a clean lift-and-shift. Three issues need to be addressed before merging.
Blocking issues
1. packages/runtime imports from apps/action/src/ via relative ../../../../src/ paths — breaks package isolation
Three new files in the runtime package reference the root src/ directly via relative paths:
packages/runtime/src/agent/setup-adapter.ts:1—import type {SetupInputs} from '../../../../src/services/setup/types.js'packages/runtime/src/agent/server.ts:2— sameSetupInputsimportpackages/runtime/src/session/logical-key.ts:1—import type {TriggerContext} from '../../../../src/features/triggers/types.js'packages/runtime/src/session/logical-key.test.ts:1—TriggerContextandGitHubContextfrom../../../../src/services/github/types.js
The PR description says "No backwards dependency: packages/runtime/ never imports from apps/action/" and that this was grep-verified. That claim is false — the relative paths cross the package boundary into the root src/ tree. SetupInputs and TriggerContext need to either be moved into packages/runtime/src/ or the types need to be duplicated/redefined there. This is the key constraint of the extraction and it is not met.
2. getRunnerOS() in packages/runtime/src/shared/env.ts has a missing return path
The function is typed (): string but the switch (platform) has no default case. If os.platform() returns anything other than the explicitly listed values (e.g. 'openbsd'... wait, 'openbsd' is listed, but the point stands: TypeScript's NodeJS.Platform type includes additional values and future Node additions). More importantly, the current code is already missing the exhaustive default — the function implicitly returns undefined for any unlisted platform, violating its return type. This is a type-safety bug that TypeScript may not catch if strict mode is off on this particular file, but it will produce a runtime undefined where a string is expected.
3. RETRY_DELAYS_MS is defined twice with different values — will cause silent retry behavior mismatch
packages/runtime/src/shared/constants.ts:[30_000, 60_000, 120_000]packages/runtime/src/agent/retry.ts:[5_000, 15_000, 30_000, 60_000]
execution.ts imports from retry.ts, so the shorter delays apply. But any future consumer that imports from shared/constants.ts will get different behavior. One of these is dead or wrong. The constants.ts version matches the older "shared constants" intent but is unused; the retry.ts version is the live one. Remove the duplicate from constants.ts or alias it, with a comment explaining the distinction.
Non-blocking concerns
Dead code in buildTaskSection (prompt.ts)
The if (appendMode) { lines.push(directive) } else { lines.push(directive) } block (both branches are identical) is dead. The appendMode field is read and destructured but the branching does nothing — directive is pushed unconditionally either way. This appears to be a refactor residue. The appendMode flag is still used correctly at the call site in buildAgentPrompt to decide whether to wrap user_supplied_instructions, so the field itself is not dead, but the if/else in buildTaskSection is.
Security comments removed from content-sync.ts and validation.ts
The CWE-377/378 comment explaining why mkdtemp is used (race condition / information disclosure on shared runners) was deleted from content-sync.ts. Similarly, the SSRF/IAM comment above hasMetadataServiceAddress was removed from validation.ts. The protective code itself remains, but the rationale is gone. These comments existed to prevent future engineers from "simplifying" the code away. Not blocking, but worth restoring.
normalizeWorkspacePath inlined in discovery.ts with a behavior change
The original used path.resolve() / path.normalize() from shared/paths.js. The inlined version uses new URL('file://' + workspacePath).pathname. On Windows this will mangle drive-letter paths (C:\foo → /C:/foo). Given this runs on GitHub-hosted Linux runners the practical risk is low, but it's a non-obvious behavioral shift that should be documented if intentional.
PruningConfig removed from session/index.ts exports
PruningConfig was previously exported; it no longer is. If any external consumer (outside this PR's diff) references it by name from the session index, this is a silent breaking change. The internal DEFAULT_PRUNING_CONFIG is still exported so callers using only the constant are unaffected, but callers importing the type for annotation purposes will break.
Missing tests
getRunnerOS()inenv.ts— no test covers the fallback branch (the function introduced in this PR, not in an existing test file). Given it has a latent type-safety bug (missingdefault/return), a test that asserts the return type and covers the fallback would catch the issue.ensureOpenCodeAvailable()inserver.ts— newly extracted with no tests.executeOpenCode()retry logic inexecution.ts— the loop withMAX_LLM_RETRIESandRETRY_DELAYS_MS(fromretry.ts) is untested. The separateRETRY_DELAYS_MSinconstants.ts(dead) further increases confusion here.
Risk assessment: MED
The cross-package import violations mean the stated isolation guarantee is not actually enforced, which is the primary goal of this extraction. If Unit 2 or the Gateway builds depend on @fro-bot/runtime being truly free of apps/action/src/ types, those builds will fail or produce circular-dependency warnings. The retry constant duplication is a low-blast-radius correctness issue. Everything else is contained to this package.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/agent |
| Run ID | 24696395813 |
| Cache | hit |
| Session | ses_252af84d3ffe3I63PiM4odtG8H |
- Define SetupInputs locally in packages/runtime (breaks cross-package import) - Import TriggerContext/EventType/TriggerTarget from runtime's own types - Remove GitHubContext dependency from test (TriggerContext.raw is unknown) - Add default return to getRunnerOS() for exhaustive platform handling - Remove duplicate RETRY_DELAYS_MS from constants.ts (canonical in retry.ts) - Fix dead appendMode if/else branch in buildTaskSection - Restore CWE-377/378 and SSRF/IAM security rationale comments - Use path.resolve/normalize instead of URL constructor in discovery.ts - Add test for getRunnerOS() default/unknown platform case - Clean up stale re-exports (LLM_RETRY_DELAY_MS, RETRY_DELAYS_MS)
725b75a to
f003b1e
Compare
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS
The fix commit (f003b1e) addresses all three blocking issues from the previous review:
-
Cross-package boundary violations resolved —
SetupInputsis now defined locally insetup-adapter.tswith a comment noting it mirrorsservices/setup/types.tswithout crossing the package boundary.logical-key.tsand its test now importTriggerContextfrom../agent/types.js. Zero../../../../src/imports remain inpackages/runtime/src/. -
getRunnerOS()missing return fixed —default: return platformadded, making the function exhaustive and type-correct. -
RETRY_DELAYS_MSduplicate removed —constants.tsno longer definesRETRY_DELAYS_MS; a comment points toagent/retry.tsas the canonical location.
The getRunnerOS fallback branch is also covered by a new test asserting that unknown platform strings pass through as-is.
The non-blocking concerns from the prior review (dead appendMode if/else, removed security comments, normalizeWorkspacePath behavior change, PruningConfig export removal) remain unaddressed but were not blocking. They stand as known issues for follow-up if desired.
Blocking issues
None.
Non-blocking concerns
buildTaskSectioninprompt.tsstill contains the deadif (appendMode) { lines.push(directive) } else { lines.push(directive) }— both branches are identical. TheappendModeflag is used correctly elsewhere inbuildAgentPrompt, but the if/else here is a no-op refactor residue.- Security rationale comments (CWE-377/378 in
content-sync.ts, SSRF/IAM invalidation.ts) remain removed. Code is still correct; comments existed to prevent future "simplification". PruningConfigtype no longer exported fromsession/index.ts.
Missing tests
ensureOpenCodeAvailable()inserver.ts— still no tests. Acceptable for a lift-and-shift but worth tracking.executeOpenCode()retry loop — still untested. Same note.
Risk assessment (LOW):
Package boundary constraints are now enforced. The extraction is clean, tests pass per the PR description, and the dist is rebuilt. Low regression risk.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/agent |
| Run ID | 24917825317 |
| Cache | hit |
| Session | ses_252af84d3ffe3I63PiM4odtG8H |
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS
The latest commit (389a74f) removes the stale apps/action/dist/ directory, and the full Group Y re-wiring is complete: src/features/agent/, src/harness/, and related modules now import from @fro-bot/runtime rather than the old internal paths. The package boundary is cleanly enforced end-to-end.
All three blocking issues from the original review remain fixed. New material in this iteration is clean.
Blocking issues
None.
Non-blocking concerns
tokenUsage: unknown type erasure in execution.ts / execution-adapter.ts
packages/runtime/src/agent/execution.ts types its return as tokenUsage: unknown (because AttemptResult.eventStreamResult.tokens in prompt-sender.ts is unknown). execution-adapter.ts bridges this with as Promise<AgentResult>, which casts unknown to TokenUsage | null. This is safe at runtime — the actual value flowing through is always TokenUsage | null from retry.ts's EventStreamResult — but the type erasure at the AttemptResult boundary means TypeScript can't catch a mismatch if the type diverges in future. The fix is to type AttemptResult.eventStreamResult.tokens as TokenUsage | null in prompt-sender.ts, which would remove the need for the cast.
context.raw as GitHubContext cast in issue-comment.ts
TriggerContext.raw is now unknown (widened to unknown in packages/runtime/src/agent/types.ts). issue-comment.ts casts it back to GitHubContext to call getCommentTarget(). This is correct at runtime but is a unknown → GitHubContext unsafe cast with no runtime validation. Documented and acceptable for this PR scope, but worth narrowing when TriggerContext stabilizes.
ensureOpenCodeAvailable in server-adapter.ts inlines OmoProviders shape instead of importing the type
The function parameter has an inline structural type for omoProviders rather than importing OmoProviders from @fro-bot/runtime. TypeScript will enforce structural compatibility so this is functionally safe, but if OmoProviders gains a new field the adapter will silently accept the old shape. Minor.
Dead if (appendMode) in prompt.ts — still present, unchanged from prior review.
Removed security comments in content-sync.ts and validation.ts — still absent, unchanged from prior review.
Missing tests
ensureOpenCodeAvailable()/bootstrapOpenCodeServer()inserver-adapter.ts— still no tests.executeOpenCode()retry loop inexecution.ts— still untested.
Risk assessment (LOW)
Package boundaries are fully enforced. Group Y imports are updated. The unknown tokenUsage cast is safe at runtime. No regressions expected.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/agent |
| Run ID | 24918002538 |
| Cache | hit |
| Session | ses_252af84d3ffe3I63PiM4odtG8H |
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS
Two new commits since the last review (7e433cc, 23b9856) are straightforward:
7e433cc: Pinsfast-xml-parserto>=5.7.0via pnpm overrides to resolve CVE-2026-41650. The lockfile correctly resolves to5.7.2for all consumers (@aws-sdk/xml-builder,@azure/core-xml). The previous override floor (>=5.5.7) is superseded. Security fix is correctly applied.23b9856: Rebuilds dist artifacts. No source changes.
All prior PASS findings hold. No new issues introduced.
Blocking issues
None.
Non-blocking concerns
Carried from prior review — none resolved, none regressed:
tokenUsage: unknowntype erasure inexecution.ts/execution-adapter.ts(safe at runtime, but theas Promise<AgentResult>cast hides future type mismatches)context.raw as GitHubContextunsafe cast inissue-comment.tsensureOpenCodeAvailableinserver-adapter.tsinlinesOmoProvidersshape rather than importing the type- Dead
if (appendMode)if/else inprompt.ts - Removed security rationale comments in
content-sync.tsandvalidation.ts
Missing tests
ensureOpenCodeAvailable()/bootstrapOpenCodeServer()inserver-adapter.tsexecuteOpenCode()retry loop inexecution.ts
Risk assessment (LOW)
CVE fix is contained to a transitive dependency pin with no API surface change. Build artifacts are a routine rebuild. No regression risk.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/agent |
| Run ID | 24918927992 |
| Cache | hit |
| Session | ses_252af84d3ffe3I63PiM4odtG8H |
…ck (#634) * docs(plans): reconcile statuses against shipped reality Four plans previously marked 'active' have shipped: - agent-cohesion-session-continuity (deterministic session titles + buildLogicalKey) - compounding-wiki (vault, schedule, seed pages via PRs #489 #491 #494) - manual-delivery-mode (output-mode input via PR #517) - gateway-discord-v1 Units 1-3 (PRs #541 #547 #548) Gateway v1 plan stays active with Units 4-8 unshipped. * ci(release): fall back to main's tree on unresolvable merge conflicts The reset-and-merge step uses 'git merge --no-ff -Xtheirs origin/main' to synthesize next from the last release tag. -Xtheirs handles content conflicts but cannot resolve rename/rename conflicts, which fire every time main's bundle artifact hash changes (dist/artifact-*.js). On conflict, take main's tree verbatim via 'git checkout origin/main -- .' and commit it as the merge result. The release branch's purpose is to mirror main; biasing fully to main on conflicts preserves that intent without manual intervention.
Summary
Extracts the reusable agent runtime into a new
packages/runtime/workspace package (@fro-bot/runtime) as Unit 1 of the Gateway v1 plan (docs/plans/2026-04-18-001-feat-fro-bot-gateway-discord-v1-plan.md).What moved
Group X →
packages/runtime/src/src/services/session/*— session storage, search, pruning, writeback, typessrc/services/object-store/*— S3-compatible durable storage adaptersrc/features/agent/{execution,prompt,prompt-thread,prompt-sender,reference-files,retry,output-mode,server,types,index}.ts— core agent executionsrc/shared/{logger,types,async,constants,env,errors,format,console,paths}.ts— pure utilitiessrc/features/comments/{types,error-format}.ts— comment type surfaceGroup Y stays in
apps/action/src/agent/{reactions,context,diff-context,streaming}.ts— GitHub-specific glueharness/,services/{github,cache,setup}/— action-only infrastructureKey constraints enforced
@actions/*imports underpackages/runtime/src/(grep-verified)packages/runtime/never imports fromapps/action/(grep-verified)SetupAdapterinterface injected intoserver.ts— runtime doesn't depend directly onservices/setup/@fro-bot/runtimebundled into action outputBuild config fix
Root
tsdown.config.tsrestored to full config (the re-export-to-.jsapproach brokepackages/runtimebuilds).packages/runtime/tsdown.config.tsadded so the runtime build doesn't inherit the action's license-collector plugin.