Add relayfile mirror mount backend#242
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a local mirror mount client (LocalMirrorMountClient) to support the relayfile-mirror backend, alongside configuration schema updates, factory orchestrator integration, and writeback verification enhancements. The review feedback highlights several key improvement opportunities, including optimizing directory subscription polling on platforms that support recursive watching, normalizing lookup keys to prevent mismatches, wrapping JSON parsing in try-catch blocks to avoid raw crashes on malformed metadata, and optimizing outbox matching performance. Additionally, it is recommended to reduce the hardcoded 90-second timeout in verifyAcked to prevent blocking during verification queries.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| try { | ||
| watcher = watch(this.mirrorDir, { recursive: true }) | ||
| startPolling() | ||
| void (async () => { | ||
| try { | ||
| for await (const event of watcher) { | ||
| if (!active) break | ||
| if (event.eventType !== 'change' && event.eventType !== 'rename') continue | ||
| if (!event.filename) continue | ||
| const relativePath = event.filename.toString() | ||
| if (!relativePath || relativePath.split(/[\\/]/u).at(0) === '.relay') continue | ||
| emitPath(toRemotePath(this.mirrorDir, resolve(this.mirrorDir, relativePath))) | ||
| } | ||
| } catch { | ||
| if (active) startPolling() | ||
| } | ||
| })() | ||
| } catch { | ||
| startPolling() | ||
| } |
There was a problem hiding this comment.
On macOS and Windows, fs.promises.watch fully supports recursive directory watching. Starting the heavy polling loop (startPolling()) concurrently with watch is redundant and leads to unnecessary CPU and disk I/O overhead (since polling recursively walks the entire directory tree and stats every file every second). Polling should only be used as a fallback on platforms where recursive watching is unsupported (like Linux) or if the watcher fails to initialize.
try {
watcher = watch(this.mirrorDir, { recursive: true })
if (process.platform !== 'darwin' && process.platform !== 'win32') {
startPolling()
}
void (async () => {
try {
for await (const event of watcher) {
if (!active) break
if (event.eventType !== 'change' && event.eventType !== 'rename') continue
if (!event.filename) continue
const relativePath = event.filename.toString()
if (!relativePath || relativePath.split(/[\\/]/u).at(0) === '.relay') continue
emitPath(toRemotePath(this.mirrorDir, resolve(this.mirrorDir, relativePath)))
}
} catch {
if (active) startPolling()
}
})()
} catch {
startPolling()
}| this.#lastWriteByPath.set(path, { | ||
| path: normalizeRemotePath(path), | ||
| localPath, | ||
| contentHash: hashString(serialized), | ||
| startedAtMs: Date.now(), | ||
| ackedBaseline: outboxCount(metadata.state, 'acked'), | ||
| }) |
There was a problem hiding this comment.
To ensure consistency and avoid potential lookup mismatches due to leading slashes or formatting, the key used in this.#lastWriteByPath should be normalized using normalizeRemotePath(path) rather than using the raw path directly.
const normalizedPath = normalizeRemotePath(path)
this.#lastWriteByPath.set(normalizedPath, {
path: normalizedPath,
localPath,
contentHash: hashString(serialized),
startedAtMs: Date.now(),
ackedBaseline: outboxCount(metadata.state, 'acked'),
})| const state = asRecord(JSON.parse(stateRaw)) ?? {} | ||
| const pidRecord = parsePidFile(pidRaw) | ||
| const pid = pidRecord.pid |
There was a problem hiding this comment.
If state.json or mount.pid is concurrently modified, corrupted, or empty, JSON.parse will throw a raw SyntaxError which crashes the operation without a clear context. Wrapping the parsing in a try/catch block ensures we can provide a descriptive error message indicating that the writeback is not ready due to a malformed metadata file.
let state: Record<string, unknown>
try {
state = asRecord(JSON.parse(stateRaw)) ?? {}
} catch (error) {
throw new Error(`Relayfile mirror writeback is not ready: failed to parse ${statePath}: ${errorMessage(error)}`)
}
const pidRecord = parsePidFile(pidRaw)| const parsePidFile = (raw: string): Record<string, unknown> & { pid: number } => { | ||
| const trimmed = raw.trim() | ||
| const parsed = trimmed.startsWith('{') ? asRecord(JSON.parse(trimmed)) : { pid: Number(trimmed) } | ||
| return { ...(parsed ?? {}), pid: Number(parsed?.pid) } | ||
| } |
There was a problem hiding this comment.
If the mount.pid file contains malformed JSON, JSON.parse will throw a raw SyntaxError and crash the application. Wrapping the parsing logic in a try/catch block and returning a fallback object with pid: 0 allows the caller to handle it gracefully and throw a clear, descriptive error.
const parsePidFile = (raw: string): Record<string, unknown> & { pid: number } => {
const trimmed = raw.trim()
try {
const parsed = trimmed.startsWith('{') ? asRecord(JSON.parse(trimmed)) : { pid: Number(trimmed) }
return { ...(parsed ?? {}), pid: Number(parsed?.pid) }
} catch {
return { pid: 0 }
}
}| for (const filePath of files) { | ||
| const raw = await readFile(filePath, 'utf8').catch(() => '') | ||
| if (outboxRecordMatches(raw, path, marker)) return true | ||
| } |
There was a problem hiding this comment.
Since confirmWrite polls outboxHasMatch repeatedly in a tight loop, reading and parsing every single file in the outbox can become a major I/O bottleneck as the outbox grows. Since we only care about writes initiated during the current operation, we can optimize this by checking the file's modification time (mtimeMs) and skipping any files that are older than marker.startedAtMs.
| for (const filePath of files) { | |
| const raw = await readFile(filePath, 'utf8').catch(() => '') | |
| if (outboxRecordMatches(raw, path, marker)) return true | |
| } | |
| for (const filePath of files) { | |
| if (marker) { | |
| const fileStat = await stat(filePath).catch(() => null) | |
| if (fileStat && fileStat.mtimeMs < marker.startedAtMs) { | |
| continue | |
| } | |
| } | |
| const raw = await readFile(filePath, 'utf8').catch(() => '') | |
| if (outboxRecordMatches(raw, path, marker)) return true | |
| } |
| const verifyAcked = async (mount: MountClient, path: string): Promise<boolean> => | ||
| await mount.confirmWrite(path, { timeoutMs: 90_000 }) === 'acked' |
There was a problem hiding this comment.
Hardcoding a 90-second timeout in verifyAcked means any call to verify on an unconfirmed or failed write will block the entire execution flow for up to 90 seconds. Since confirmWriteback already waits for the write to be acked before calling verify, this second wait is completely redundant. We should allow verifyAcked to accept an optional timeout that defaults to 0 (immediate check) to prevent blocking during verification queries.
| const verifyAcked = async (mount: MountClient, path: string): Promise<boolean> => | |
| await mount.confirmWrite(path, { timeoutMs: 90_000 }) === 'acked' | |
| const verifyAcked = async (mount: MountClient, path: string, timeoutMs = 0): Promise<boolean> => | |
| await mount.confirmWrite(path, { timeoutMs }) === 'acked' |
|
Warning Review limit reached
More reviews will be available in 2 minutes and 35 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces a filesystem-backed local mirror mount client for the Factory SDK, allowing configuration-driven selection between a cloud-based and local mirror backend. It includes the new client implementation with read/write/subscribe operations, comprehensive tests, automatic mount construction in the orchestrator, and enhanced write verification using ack confirmation. ChangesLocal Mirror Mount Backend
Sequence DiagramsequenceDiagram
participant FactoryConfig
participant FactoryLoop
participant createMount
participant LocalMirrorMountClient
participant Filesystem
participant MountLinearWriteback
FactoryConfig->>FactoryLoop: config with mount backend
FactoryLoop->>createMount: config.mount (backend, mirrorDir)
alt backend === 'relayfile-mirror'
createMount->>LocalMirrorMountClient: new LocalMirrorMountClient(config)
LocalMirrorMountClient-->>createMount: instance
else default to cloud
createMount-->>FactoryLoop: RelayfileCloudMountClient
end
createMount-->>FactoryLoop: mount instance
FactoryLoop->>MountLinearWriteback: initialize with mount
MountLinearWriteback->>LocalMirrorMountClient: writeFile(path, content)
LocalMirrorMountClient->>Filesystem: persist mirror payload
LocalMirrorMountClient-->>MountLinearWriteback: write recorded
MountLinearWriteback->>LocalMirrorMountClient: confirmWrite(path, timeout)
LocalMirrorMountClient->>Filesystem: poll outbox status
alt outbox acked
LocalMirrorMountClient-->>MountLinearWriteback: 'acked'
MountLinearWriteback->>LocalMirrorMountClient: readFile(path)
LocalMirrorMountClient-->>MountLinearWriteback: content + revision
MountLinearWriteback-->>MountLinearWriteback: verify payload
else pending or timeout
LocalMirrorMountClient-->>MountLinearWriteback: 'pending' or 'timeout'
MountLinearWriteback-->>MountLinearWriteback: verify fails
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Fixed the PR issues I found:
Addressed comments
Advisory Notes
Validation run locally:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/factory-sdk/src/config/schema.test.ts (1)
62-80: ⚡ Quick winAdd test coverage for invalid mirror backend config.
The test verifies parsing when
mirrorDiris provided, but there's no test for the case wherebackend: 'relayfile-mirror'is specified withoutmirrorDir. If the schema is updated to enforcemirrorDirwhen the backend is'relayfile-mirror'(as suggested in the schema.ts review), add a corresponding negative test case.🧪 Proposed test for invalid mirror config
expect(parsed.mount).toEqual({ backend: 'relayfile-mirror', mirrorDir: '/tmp/factory-rf-mirror', }) }) + +it('rejects relayfile-mirror backend without mirrorDir', () => { + expect(() => FactoryConfigSchema.parse({ + workspaceId: 'rw_123', + mount: { + backend: 'relayfile-mirror', + }, + repos: { + byLabel: { + pear: 'AgentWorkforce/pear', + }, + }, + })).toThrow(/mirrorDir/) +})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/factory-sdk/src/config/schema.test.ts` around lines 62 - 80, Add a negative test that ensures FactoryConfigSchema.parse throws when mount.backend is 'relayfile-mirror' but mirrorDir is missing: create a test (e.g., it('rejects relayfile-mirror without mirrorDir', ...)) that calls FactoryConfigSchema.parse with workspaceId, mount: { backend: 'relayfile-mirror' } and repos, and assert the parse call throws (use expect(() => FactoryConfigSchema.parse(...)).toThrow()). This mirrors the positive test and verifies the schema validation added to FactoryConfigSchema enforces mirrorDir for the 'relayfile-mirror' backend.packages/factory-sdk/src/mount/local-mirror-mount-client.ts (1)
277-287: 💤 Low valueHeuristic-based ack detection has edge cases with concurrent writes.
The fallback at lines 280-282 assumes that if the global acked count increased and there are no pending writes, this specific write was acked. With concurrent writes from multiple processes, this could incorrectly report 'acked' for a write that's still in flight.
This is likely acceptable given the file-based coordination model, but a brief comment explaining the heuristic would help maintainability.
📝 Suggested comment
const state = await readRelayState(this.mirrorDir) if (state) { if (outboxCount(state, 'failed') > 0) return 'failed' + // Heuristic: if global acked count increased since our write started and + // there are no pending writes, assume our write was included in the batch. + // This may report 'acked' prematurely with concurrent multi-process writes. if (marker && outboxCount(state, 'acked') > marker.ackedBaseline && !hasPendingWriteback(state)) { return 'acked' }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/factory-sdk/src/mount/local-mirror-mount-client.ts` around lines 277 - 287, Add a brief explanatory comment above the heuristic fallback in the method that reads relay state (around the call to readRelayState(this.mirrorDir)) describing that the ack-detection is best-effort: it treats an increased global acked count (outboxCount(state, 'acked') > marker.ackedBaseline) and no pending writebacks (hasPendingWriteback(state) === false) as evidence this specific write was acked, but with concurrent writers this can be a false positive; note that the fallback return (return marker ? 'pending' : 'acked') is conservative for missing state and this heuristic is intentional due to file-based coordination. Reference readRelayState, outboxCount, hasPendingWriteback and marker in the comment so future maintainers can find and reason about the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/factory-sdk/src/mount/local-mirror-mount-client.ts`:
- Around line 142-148: filesystemEventToChangeEvent is returning a ChangeEvent
type that doesn't match the ChangeEvent expected by MountClient.subscribe (which
is an alias of RelayFileChangeEvent), so the code uses a double cast `as unknown
as ChangeEvent`; fix this by aligning types: either change
filesystemEventToChangeEvent's return type to the ports' ChangeEvent
(RelayFileChangeEvent) or create/export a single shared ChangeEvent type alias
(sourced from `@relayfile/sdk`) and use it in filesystemEventToChangeEvent and
MountClient.subscribe, then remove the `as unknown as ChangeEvent` cast;
alternatively implement a small adapter that constructs an object explicitly
typed as the ports' ChangeEvent (used where filesystemEventToChangeEvent is
called) to ensure type compatibility without casts.
In `@packages/factory-sdk/src/orchestrator/factory.ts`:
- Line 81: The MountGithubRead invocation in FactoryLoop is a no-op—remove the
dead expression `void (ports.github ?? MountGithubRead(mount))` or instead
assign/save the result so it can be used later; either set `ports.github =
ports.github ?? MountGithubRead(mount)` or store the returned `GithubRead` (the
object with `getPr`) on the FactoryLoop instance so later code can call `getPr`;
update references so the factory actually uses the saved `GithubRead` (or remove
any unused `MountGithubRead` import) to eliminate the dead initialization.
---
Nitpick comments:
In `@packages/factory-sdk/src/config/schema.test.ts`:
- Around line 62-80: Add a negative test that ensures FactoryConfigSchema.parse
throws when mount.backend is 'relayfile-mirror' but mirrorDir is missing: create
a test (e.g., it('rejects relayfile-mirror without mirrorDir', ...)) that calls
FactoryConfigSchema.parse with workspaceId, mount: { backend: 'relayfile-mirror'
} and repos, and assert the parse call throws (use expect(() =>
FactoryConfigSchema.parse(...)).toThrow()). This mirrors the positive test and
verifies the schema validation added to FactoryConfigSchema enforces mirrorDir
for the 'relayfile-mirror' backend.
In `@packages/factory-sdk/src/mount/local-mirror-mount-client.ts`:
- Around line 277-287: Add a brief explanatory comment above the heuristic
fallback in the method that reads relay state (around the call to
readRelayState(this.mirrorDir)) describing that the ack-detection is
best-effort: it treats an increased global acked count (outboxCount(state,
'acked') > marker.ackedBaseline) and no pending writebacks
(hasPendingWriteback(state) === false) as evidence this specific write was
acked, but with concurrent writers this can be a false positive; note that the
fallback return (return marker ? 'pending' : 'acked') is conservative for
missing state and this heuristic is intentional due to file-based coordination.
Reference readRelayState, outboxCount, hasPendingWriteback and marker in the
comment so future maintainers can find and reason about the logic.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ad9dc7ac-766a-4cd6-a9c4-07667261bc1e
📒 Files selected for processing (10)
packages/factory-sdk/src/config/schema.test.tspackages/factory-sdk/src/config/schema.tspackages/factory-sdk/src/index.tspackages/factory-sdk/src/mount/create-mount.tspackages/factory-sdk/src/mount/local-mirror-mount-client.test.tspackages/factory-sdk/src/mount/local-mirror-mount-client.tspackages/factory-sdk/src/orchestrator/factory.tspackages/factory-sdk/src/types.tspackages/factory-sdk/src/writeback/linear.tspackages/factory-sdk/src/writeback/writeback.test.ts
| onChange(filesystemEventToChangeEvent(null, this.workspaceId, { | ||
| type: 'file.updated', | ||
| path: changedPath, | ||
| eventId: `mirror:${this.workspaceId}:${changedPath}:${Date.now()}`, | ||
| revision: String(Date.now()), | ||
| timestamp: new Date().toISOString(), | ||
| }) as unknown as ChangeEvent) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the return type of filesystemEventToChangeEvent and ChangeEvent interface
echo "=== filesystemEventToChangeEvent function ==="
ast-grep --pattern 'function filesystemEventToChangeEvent($$$) { $$$ }'
ast-grep --pattern 'const filesystemEventToChangeEvent = ($$$) => $$$'
ast-grep --pattern 'export const filesystemEventToChangeEvent = ($$$) => $$$'
echo "=== ChangeEvent type definition ==="
rg -n --type=ts 'type ChangeEvent|interface ChangeEvent' -A 10Repository: AgentWorkforce/pear
Length of output: 5789
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Locate filesystemEventToChangeEvent ==="
rg -n "filesystemEventToChangeEvent" packages/factory-sdk/src || true
echo "=== Inspect local-mirror-mount-client subscribe region (around 142-148) ==="
sed -n '110,180p' packages/factory-sdk/src/mount/local-mirror-mount-client.ts
echo "=== Inspect ChangeEvent import/type usage in local-mirror-mount-client ==="
rg -n "import type \\{[^}]*ChangeEvent|from '\\.\\./ports'|from '\\./'|ChangeEvent" packages/factory-sdk/src/mount/local-mirror-mount-client.ts
echo "=== Inspect ports ChangeEvent type (context for runtime/typing) ==="
sed -n '1,120p' packages/factory-sdk/src/ports/mount.ts
echo "=== If filesystemEventToChangeEvent is defined elsewhere, print nearby definition ==="
# Best-effort: show candidate definition locations
for f in $(rg -l "filesystemEventToChangeEvent" packages/factory-sdk/src || true); do
echo "--- $f (near match) ---"
rg -n "filesystemEventToChangeEvent" "$f" -n
doneRepository: AgentWorkforce/pear
Length of output: 6705
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspect filesystemEventToChangeEvent signature/body ==="
sed -n '120,280p' packages/factory-sdk/src/subscriptions/event-client.ts
echo "=== Inspect exported filesystemEventToChangeEvent types (if any) ==="
rg -n "export function filesystemEventToChangeEvent" -n packages/factory-sdk/src/subscriptions/event-client.ts
echo "=== Inspect Subscription/Event-client emit call site types context ==="
sed -n '200,260p' packages/factory-sdk/src/subscriptions/event-client.tsRepository: AgentWorkforce/pear
Length of output: 8541
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspect ChangeEvent type in subscriptions/event-client.ts (top) ==="
sed -n '1,120p' packages/factory-sdk/src/subscriptions/event-client.ts
echo "=== Inspect subscriptions/index.ts exports for filesystemEventToChangeEvent ==="
sed -n '1,120p' packages/factory-sdk/src/subscriptions/index.ts
echo "=== Inspect ports/mount.ts ChangeEvent alias again (context) ==="
sed -n '1,60p' packages/factory-sdk/src/ports/mount.tsRepository: AgentWorkforce/pear
Length of output: 6113
Avoid as unknown as ChangeEvent by aligning the ChangeEvent types used by filesystemEventToChangeEvent and MountClient.subscribe.
filesystemEventToChangeEvent() is typed to return ChangeEvent from packages/factory-sdk/src/subscriptions/event-client.ts, while subscribe() expects ChangeEvent from packages/factory-sdk/src/ports/mount.ts (an alias of RelayFileChangeEvent from @relayfile/sdk). The as unknown as ChangeEvent cast hides this mismatch.
Consider either:
- Updating
filesystemEventToChangeEvent’s return type to match theportsChangeEvent(so the cast can be removed), or - Exporting a single shared
ChangeEventtype alias (same source as@relayfile/sdk) and using it in both places, or otherwise constructing an explicit adapter typed as theportsChangeEventwithout the double cast.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/factory-sdk/src/mount/local-mirror-mount-client.ts` around lines 142
- 148, filesystemEventToChangeEvent is returning a ChangeEvent type that doesn't
match the ChangeEvent expected by MountClient.subscribe (which is an alias of
RelayFileChangeEvent), so the code uses a double cast `as unknown as
ChangeEvent`; fix this by aligning types: either change
filesystemEventToChangeEvent's return type to the ports' ChangeEvent
(RelayFileChangeEvent) or create/export a single shared ChangeEvent type alias
(sourced from `@relayfile/sdk`) and use it in filesystemEventToChangeEvent and
MountClient.subscribe, then remove the `as unknown as ChangeEvent` cast;
alternatively implement a small adapter that constructs an object explicitly
typed as the ports' ChangeEvent (used where filesystemEventToChangeEvent is
called) to ensure type compatibility without casts.
| this.#slack = ports.slack ?? (config.slack ? MountSlackWriteback(ports.mount, config.slack) : undefined) | ||
| void (ports.github ?? MountGithubRead(ports.mount)) | ||
| this.#slack = ports.slack ?? (config.slack ? MountSlackWriteback(mount, config.slack) : undefined) | ||
| void (ports.github ?? MountGithubRead(mount)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if MountGithubRead has constructor side effects or if the result should be stored
# Search for MountGithubRead usage patterns
echo "=== MountGithubRead usage patterns ==="
rg -n "MountGithubRead" --type=ts -C2
# Search for github port references in FactoryLoop
echo "=== References to github port in factory.ts ==="
rg -n "this\.#github|ports\.github" packages/factory-sdk/src/orchestrator/factory.ts -C2
# Check if there's a github field declaration
echo "=== Github field declarations ==="
ast-grep --pattern 'class FactoryLoop {
$$$
`#github`$_
$$$
}'Repository: AgentWorkforce/pear
Length of output: 4100
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect MountGithubRead implementation
echo "=== packages/factory-sdk/src/writeback/github.ts (relevant excerpt) ==="
sed -n '1,140p' packages/factory-sdk/src/writeback/github.ts
# Inspect factory.ts around the constructor/class fields and the line in question
echo "=== packages/factory-sdk/src/orchestrator/factory.ts (around FactoryLoop init) ==="
sed -n '1,220p' packages/factory-sdk/src/orchestrator/factory.ts | nl -ba | sed -n '50,130p'
# Search for any usage of a stored github port / field in factory loop
echo "=== Search for '`#github`' in factory.ts ==="
rg -n "`#github`" packages/factory-sdk/src/orchestrator/factory.ts -S || true
# Search the repo for ports.github and 'MountGithubRead(mount' usage
echo "=== Search repo for ports.github ==="
rg -n "ports\.github" --type=ts packages/factory-sdk/src || true
echo "=== Search repo for MountGithubRead(mount) ==="
rg -n "MountGithubRead\(mount\)" --type=ts packages/factory-sdk/src || true
echo "=== Search repo for '.github' on returned objects named ports ==="
rg -n "github.*Mount|MountGithubRead|getPr\(" --type=ts packages/factory-sdk/src/orchestrator packages/factory-sdk/src | head -n 50Repository: AgentWorkforce/pear
Length of output: 2571
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect MountGithubRead implementation
echo "=== packages/factory-sdk/src/writeback/github.ts (relevant excerpt) ==="
sed -n '1,160p' packages/factory-sdk/src/writeback/github.ts
# Inspect factory.ts around the constructor/class fields and the line in question
echo "=== packages/factory-sdk/src/orchestrator/factory.ts (around FactoryLoop init) ==="
sed -n '1,220p' packages/factory-sdk/src/orchestrator/factory.ts | nl -ba | sed -n '60,120p'
# Search for any usage of a stored github port / field in factory loop
echo "=== Search for '`#github`' in factory.ts ==="
rg -n "`#github`" packages/factory-sdk/src/orchestrator/factory.ts -S || true
# Search the repo for ports.github and other MountGithubRead call patterns
echo "=== Search repo for ports.github ==="
rg -n "ports\.github" --type=ts packages/factory-sdk/src || true
echo "=== Search repo for 'MountGithubRead(mount' ==="
rg -n "MountGithubRead\(mount" --type=ts packages/factory-sdk/src || trueRepository: AgentWorkforce/pear
Length of output: 2571
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect MountGithubRead implementation
echo "=== packages/factory-sdk/src/writeback/github.ts (relevant excerpt) ==="
sed -n '1,160p' packages/factory-sdk/src/writeback/github.ts
# Inspect factory.ts around the constructor/class fields and the line in question
echo "=== packages/factory-sdk/src/orchestrator/factory.ts (around FactoryLoop init) ==="
sed -n '1,220p' packages/factory-sdk/src/orchestrator/factory.ts | nl -ba | sed -n '60,120p'
# Search for any usage of a stored github port / field in factory loop
echo "=== Search for '`#github`' in factory.ts ==="
rg -n "`#github`" packages/factory-sdk/src/orchestrator/factory.ts -S || true
# Search the repo for ports.github and other MountGithubRead call patterns
echo "=== Search repo for ports.github ==="
rg -n "ports\.github" --type=ts packages/factory-sdk/src || true
echo "=== Search repo for 'MountGithubRead(mount' ==="
rg -n "MountGithubRead\(mount" --type=ts packages/factory-sdk/src || trueRepository: AgentWorkforce/pear
Length of output: 2571
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== factory.ts around the init line (lines ~60-110) ==="
sed -n '60,110p' packages/factory-sdk/src/orchestrator/factory.ts
echo "=== Search for '`#github`' in factory.ts ==="
rg -n "`#github`" packages/factory-sdk/src/orchestrator/factory.ts -S || true
echo "=== Search repo for 'ports.github' ==="
rg -n "ports\.github" --type=ts packages/factory-sdk/src || true
echo "=== Search repo for 'MountGithubRead(mount' ==="
rg -n "MountGithubRead\(mount" --type=ts packages/factory-sdk/src || true
echo "=== Search repo for 'MountGithubRead' call sites ==="
rg -n "MountGithubRead" --type=ts packages/factory-sdk/src || trueRepository: AgentWorkforce/pear
Length of output: 3069
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for getPr usage in orchestrator ==="
rg -n "\.getPr\(" packages/factory-sdk/src/orchestrator --type=ts || true
echo "=== Search for github usage keywords in orchestrator ==="
rg -n "github" packages/factory-sdk/src/orchestrator --type=ts || true
echo "=== Locate FactoryPorts type/interface ==="
rg -n "type FactoryPorts|interface FactoryPorts|FactoryPorts" packages/factory-sdk/src/orchestrator/factory.ts packages/factory-sdk/src/orchestrator -S --type=ts || true
echo "=== Show FactoryPorts definition (if in types) ==="
fd -t f "types.ts" packages/factory-sdk/src/orchestrator packages/factory-sdk/src --full-path | head -n 20
rg -n "FactoryPorts" packages/factory-sdk/src --type=ts || trueRepository: AgentWorkforce/pear
Length of output: 2434
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== packages/factory-sdk/src/types.ts (FactoryPorts) ==="
sed -n '1,220p' packages/factory-sdk/src/types.ts
echo "=== Search entire repo for 'ports.github' property usage ==="
rg -n "ports\.github" --type=ts || true
echo "=== Search entire repo for '`#github`' private fields ==="
rg -n "`#github`" --type=ts packages/factory-sdk/src || true
echo "=== Search entire repo for '.github' access on FactoryLoop/factory instances ==="
rg -n "\.github\b" --type=ts packages/factory-sdk/src/orchestrator || trueRepository: AgentWorkforce/pear
Length of output: 3875
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== factory.ts: search for GithubRead/getPr/GithubRead-like usage ==="
rg -n "GithubRead|getPr|MountGithubRead|github\." packages/factory-sdk/src/orchestrator/factory.ts --type=ts -S || true
echo "=== factory.ts: inspect around githubPullRoot usage (~620-710) ==="
sed -n '600,720p' packages/factory-sdk/src/orchestrator/factory.tsRepository: AgentWorkforce/pear
Length of output: 4669
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Locate GithubRead type in packages/factory-sdk/src/ports ==="
fd -t f "ports.ts" packages/factory-sdk/src | head -n 20
fd -t f "ports*.ts" packages/factory-sdk/src | head -n 50
# Prefer exact file if present
if [ -f packages/factory-sdk/src/ports.ts ]; then
sed -n '1,220p' packages/factory-sdk/src/ports.ts
fi
# Otherwise search for GithubRead definition
rg -n "export (type|interface) GithubRead" packages/factory-sdk/src --type=ts || true
rg -n "GithubRead" packages/factory-sdk/src/ports --type=ts || trueRepository: AgentWorkforce/pear
Length of output: 339
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== packages/factory-sdk/src/ports/writeback.ts (GithubRead) ==="
sed -n '1,200p' packages/factory-sdk/src/ports/writeback.ts
echo "=== packages/factory-sdk/src/ports/index.ts (exports) ==="
sed -n '1,120p' packages/factory-sdk/src/ports/index.ts
echo "=== Search for GithubRead usage across src ==="
rg -n "GithubRead" packages/factory-sdk/src --type=ts || true
echo "=== Search for getPr() call sites across src ==="
rg -n "\.getPr\(" packages/factory-sdk/src --type=ts || trueRepository: AgentWorkforce/pear
Length of output: 2723
Remove the dead MountGithubRead initialization in FactoryLoop
packages/factory-sdk/src/orchestrator/factory.ts constructor contains void (ports.github ?? MountGithubRead(mount)) and then never stores or uses a GithubRead port. MountGithubRead just returns { getPr } without side effects during construction, so this line is effectively a no-op; remove it or store ports.github/the result in a field if the factory is meant to call getPr later.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/factory-sdk/src/orchestrator/factory.ts` at line 81, The
MountGithubRead invocation in FactoryLoop is a no-op—remove the dead expression
`void (ports.github ?? MountGithubRead(mount))` or instead assign/save the
result so it can be used later; either set `ports.github = ports.github ??
MountGithubRead(mount)` or store the returned `GithubRead` (the object with
`getPr`) on the FactoryLoop instance so later code can call `getPr`; update
references so the factory actually uses the saved `GithubRead` (or remove any
unused `MountGithubRead` import) to eliminate the dead initialization.
|
Closing: superseded by the empirical writeback finding. The LocalMirrorMountClient (raw-fs write to the local mirror) is not the headless writeback mechanism — the relayfile mirror local-fs is read-only (ACL workspace-API-managed), and more importantly the real path is simpler: a cloud-API write (W8 RelayfileCloudMountClient client.writeFile, workspace creds) is pushed to the provider by the relayfile CLOUD BACKEND's own WritebackConsumer (proven: created AR-133 end-to-end, Pear down, no mount-write, no own-consumer). So no local-mirror client, no own-consumer, no re-mount needed. The diligent fail-closed/acked-not-queued/guards-above-mount review work here directly informed the final design (per-opId providerResult verify + the chokepoint backstop). Follow-up = W10 (writeback payload hygiene: strip read-only fields + verify-repoint + is-real-issue dispatch guard + structural invariant audit + cloud-client chokepoint). |
|
Implemented fixes for the PR and reran the repo’s CI-equivalent local checks. Changed:
Addressed comments
Advisory NotesNone. VerificationPassed locally:
|
1 participant
Summary
LocalMirrorMountClientfor relayfile mirror filesystem reads, writes, listing, subscriptions, event fallback, and writeback ACK confirmation.createMountplusconfig.mount.backendselection forrelayfile-cloudandrelayfile-mirror;createFactorycan construct the mirror backend when the mount port is omitted.MountLinearWriteback.verifyso a local read-back match is not considered done unless the mount reports ACKED.Tests
npx vitest run packages/factory-sdknpx tsc --noEmit -p tsconfig.node.jsonNotes
src/mainedits..relay/state.json/mount.pidare missing, stale/offline, misconfigured, or the mount PID is not alive.