fix(hub,cli): keep inactive file previews available through runner - #1727
techotaku39 wants to merge 6 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
-
[Major] Bound machine-scoped file reads before encoding — the new runner handler reads any requested path in full and only then base64-encodes it. A large regular file can exhaust the long-lived runner, and a payload above the 48 MiB Socket.IO limit cannot be returned; non-regular files such as FIFOs can also leave the RPC hanging. Evidence:
cli/src/modules/common/workspaceFileHandlers.ts:177.Suggested fix:
import { readBoundedRegularFile } from './generatedImages' const MAX_WORKSPACE_FILE_BYTES = 25 * 1024 * 1024 const buffer = await readBoundedRegularFile( resolved.path, MAX_WORKSPACE_FILE_BYTES, ) const stats = await stat(resolved.path)
-
[Minor] Preserve the recorded workspace path verbatim — trimming
cwdchanges valid Unix directory names ending in whitespace, so an archived session for/work/projectcan be redirected to/work/projector fail against the wrong path. Evidence:cli/src/modules/common/workspaceFileHandlers.ts:82.Suggested fix:
if (typeof value !== 'string' || value.length === 0) { return { error: 'Workspace path is required' } } const cwd = await pathPolicy.resolveForCheck(value)
Questions
- None.
Summary
Review mode: initial
Two issues found in the new machine-scoped workspace fallback: unsafe runner-wide file reads and path mutation for whitespace-suffixed workspaces.
Testing
Not run (automation; PR content was not executed). Add coverage for oversized/non-regular file rejection and a workspace path ending in whitespace. GitHub integration and drift-gate checks passed; test was still in progress during review.
HAPI Bot
|
|
||
| try { | ||
| const stats = await stat(resolved.path) | ||
| const buffer = await readFile(resolved.path) |
There was a problem hiding this comment.
[MAJOR] Bound machine-scoped file reads before encoding
This new long-lived runner path calls readFile without a regular-file check or byte limit. A large workspace file is allocated in full, expanded again as base64, and anything above the hub's 48 MiB Socket.IO envelope cannot complete; a FIFO can also leave the RPC pending indefinitely. This affects the runner process shared by inactive-session access, not just a disposable session CLI.
Suggested fix:
import { readBoundedRegularFile } from './generatedImages'
const MAX_WORKSPACE_FILE_BYTES = 25 * 1024 * 1024
const buffer = await readBoundedRegularFile(
resolved.path,
MAX_WORKSPACE_FILE_BYTES,
)
const stats = await stat(resolved.path)Please add tests for an oversized regular file and a FIFO/non-regular path.
| ): Promise<ResolvedWorkspaceCwd> { | ||
| const raw = typeof value === 'string' ? value.trim() : '' | ||
| if (!raw) { | ||
| return { error: 'Workspace path is required' } |
There was a problem hiding this comment.
[MINOR] Preserve the recorded workspace path verbatim
cwd comes from stored session metadata, so trimming it changes valid Unix paths. An archived session rooted at /work/project will resolve /work/project instead and can display the wrong workspace when that sibling exists.
Suggested fix:
if (typeof value !== 'string' || value.length === 0) {
return { error: 'Workspace path is required' }
}
const cwd = await pathPolicy.resolveForCheck(value)Add a containment/read test using a workspace directory whose name ends in whitespace.
There was a problem hiding this comment.
Findings
- None.
Summary
Review mode: follow-up after new commits
No issues found in the current diff. The prior findings on unbounded/non-regular file reads and whitespace-suffixed workspace paths are resolved in the latest commit. Residual risk: Windows-specific special-file behavior is not covered by the FIFO test.
Testing
Not run (automation; PR content was not executed). The PR reports typecheck, focused CLI/hub/shared tests, build, and smoke coverage. Current GitHub checks were inspected before submission.
HAPI Bot
heavygee
added
area:cli
…ner-fallback # Conflicts: # cli/src/api/apiMachine.ts
There was a problem hiding this comment.
Findings
- [Major] New Runner Git RPCs can expose files outside the allowed workspace —
cli/src/modules/common/workspaceFileHandlers.ts:305; related aggregate commands at lines 266 and 284. Reproduced Git's sibling-file traversal and:(top)pathspec bypass in an isolated fixture. Evidence and concrete fix attached inline.
Summary
Review mode: follow-up after new commits
Reviewed the full current diff. One workspace-containment issue found in the machine-scoped Git fallback.
Testing
PR tests not run (automation; PR code was not executed). Independently verified Git behavior and the proposed literal/scoped argument changes using synthetic files. Add nested-workspace containment tests for staged and unstaged Git operations; current coverage uses the repository root as the workspace.
HAPI Bot
| } | ||
|
|
||
| const args = data.staged | ||
| ? ['diff', '--cached', '--no-ext-diff', '--', filePath] |
There was a problem hiding this comment.
[Major] Enforce workspace containment in Git's pathspecs
With both the session workspace and Runner allowlist set to /repo/allowed, filePath = ":(top)outside.txt" passes resolveWorkspacePath: filesystem resolution treats it as a nonexistent child of the allowed directory. Git instead interprets it relative to /repo and returns the diff of /repo/outside.txt. The authenticated git-diff-file route forwards this path unchanged. This bypasses the new Runner's workspace boundary after the session CLI exits. The status and numstat commands at cli/src/modules/common/workspaceFileHandlers.ts:266 and cli/src/modules/common/workspaceFileHandlers.ts:284 also include sibling files because setting cwd does not scope Git's repository traversal.
Suggested fix: disable pathspec magic for file requests and explicitly scope aggregate requests to the workspace:
// WorkspaceGitStatus arguments:
['status', '--porcelain=v2', '--branch', '--untracked-files=all', '--', '.']
// WorkspaceGitDiffNumstat arguments:
['diff', ...(data.staged ? ['--cached'] : []), '--numstat', '--', '.']
// WorkspaceGitDiffFile arguments:
['--literal-pathspecs', 'diff', ...(data.staged ? ['--cached'] : []),
'--no-ext-diff', '--', filePath]Add staged/unstaged regression cases with an allowed workspace below the repository root, a changed sibling file, and :(top)/exclusion pathspec inputs. Existing Git coverage initializes the repository at the workspace root, so it misses this boundary.
There was a problem hiding this comment.
Findings
No actionable issues found in the full latest PR diff.
Summary
Review mode: follow-up after new commits
The previously reported Git workspace-containment issue is addressed at cli/src/modules/common/workspaceFileHandlers.ts:266, :284, and :305. No additional high-confidence findings. Residual coverage gaps: namespace/machine mismatch and missing fallback-handler cases in hub/src/sync/syncEngineWorkspaceFallback.test.ts; symlink escapes in cli/src/modules/common/workspaceFileHandlers.test.ts.
Testing
PR tests not run (automation; PR code was not executed). Independent synthetic Git checks passed for nested-workspace status, staged/unstaged numstat and file diffs, and top/exclusion pathspec isolation. Add regression cases for the coverage gaps above.
HAPI Bot
…ner-fallback # Conflicts: # hub/src/sync/syncEngine.ts
There was a problem hiding this comment.
Review mode: follow-up after new commits
Requirement — Pass
Keeping current workspace previews available after the session CLI disconnects is useful and addresses a verified availability gap when the Runner remains online.
Evidence
- The PR description explicitly scopes the feature to Runner-backed access without archive snapshots.
- Issue #1536 documents offline file-preview failures; base hub/src/sync/syncEngine.ts:3960 delegates file reads exclusively to session RPCs.
Approach — Pass
The optional machine capability and fallback on missing session RPC targets provide a viable solution. The hub checks inactivity, namespace, and exact machine identity; the Runner enforces workspace boundaries.
Evidence
- hub/src/sync/syncEngine.ts:298 gates fallback on session inactivity and the matching online Runner's capability.
- cli/src/modules/common/workspaceFileHandlers.ts:95 resolves paths and checks Runner roots and session-workspace containment.
- shared/src/runnerCapabilities.ts:43 advertises the capability without making it mandatory.
Code — Reviewed
Reviewed the full 16-file merge-base diff at the fixed head. Found one minor path-validation defect.
- [Minor] Allow valid workspace names beginning with two dots —
cli/src/modules/common/workspaceFileHandlers.ts:75
Testing
Not run (automation; PR code execution is prohibited).
- PR code, tests, builds, and scripts were not executed.
- Fixed-head CI: drift-gate, integration, and windows-codex-mcp succeeded; test remained in progress when checked.
- The PR reports passing local typechecks, focused tests, builds, and smoke tests; these were not independently reproduced.
- Added tests cover fallback routing and basic containment, but omit symlink escapes, namespace/machine mismatches, and missing fallback handlers.
HAPI Bot
|
|
||
| function isWithinRoot(root: string, target: string): boolean { | ||
| const child = relative(root, target) | ||
| return child === '' || (!child.startsWith('..') && !isAbsolute(child)) |
There was a problem hiding this comment.
[Minor] Allow valid workspace names beginning with two dots
For an in-workspace file such as /workspace/project/..notes.txt, relative(root, target) returns '..notes.txt'. The startsWith('..') check incorrectly classifies it as an escape. Directory listing exposes this file, but opening it through the inactive-session fallback fails with 'Path is outside the session workspace'. Directories with this prefix are similarly inaccessible.
Suggested fix
Reject only '..' itself and paths beginning with '..' followed by the platform separator, retaining the absolute-path check. Use the same predicate as cli/src/api/machinePathPolicy.ts:34 and add coverage for valid '..notes.txt' and '..assets/file.txt' paths.
There was a problem hiding this comment.
Review mode: follow-up after new commits
Requirement — Pass
Keeping current workspace previews available after a session disconnects addresses a verified availability gap when its Runner remains online.
Evidence
- The PR description explicitly requests Runner-backed access without archive snapshots.
- Issue #1536 confirms offline file-preview failures; base hub/src/sync/syncEngine.ts:3960 delegates file reads exclusively to session RPCs.
Approach — Pass
An optional Runner capability and fallback for missing session RPC targets provide a viable solution while preserving the primary session path and workspace restrictions.
Evidence
- hub/src/sync/syncEngine.ts:298 requires an inactive session and the matching online machine in its namespace; line 315 limits fallback to RpcTargetMissingError.
- cli/src/modules/common/workspaceFileHandlers.ts:95 canonicalizes paths and checks both Runner roots and session-workspace containment.
- shared/src/runnerCapabilities.ts:44 advertises workspace-file-access without making it mandatory.
Code — Reviewed
Reviewed the full 16-file merge-base diff at 51084ca. No actionable defects identified. The previous two-dot filename rejection is fixed, with regression coverage. Runtime behavior was not independently tested.
No reportable code issues found.
Testing
Not run (automation; PR code execution is prohibited).
- PR code, tests, builds, and scripts were not executed, as required.
- Fixed-head CI: integration, windows-codex-mcp, and drift-gate succeeded; test remained in progress when checked.
- Added tests cover fallback routing, capability/activity gates, basic containment, bounded reads, nested-workspace Git restrictions, and valid filenames beginning with two dots.
- Added tests do not directly cover symlink escapes, namespace/machine mismatches, or missing fallback handlers.
- The PR reports passing local typechecks, focused tests, builds, and smoke tests; these results were not independently reproduced.
HAPI Bot
This branch has not been deployed
2 participants
Summary
workspace-file-accessmachine capability for long-lived Runners.Validation
bun typecheck— passed.pwsh -NoProfile -File .\scripts\Invoke-HapiTaskPlaywright.ps1 -Name archived-file-runner-fallback -Suite Root -TestArgs terminal-wrap-fidelity.spec.ts— 2/2 passed.cd cli && bun run test -- src/modules/common/workspaceFileHandlers.test.ts src/api/apiMachine.test.ts src/agent/sessionFactory.test.ts— 36/36 passed.cd hub && bun test src/sync/syncEngineWorkspaceFallback.test.ts src/sync/rpcGateway.test.ts src/web/routes/git.test.ts— 24/24 passed.cd shared && bun test src/runnerCapabilities.test.ts— 4/4 passed.bun run build— passed.git diff --check— passed.Related Issues
Refs #1536
AI Disclosure
Implemented with assistance from OpenAI Codex using GPT-5.6.