Repository navigation
feat(open): add complete-file viewer and lazy directory browser - #1148
benvinegar wants to merge 8 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const timeout = setTimeout(abort, 1500); | ||
| let ignored: Set<string>; | ||
| try { | ||
| ignored = new Set((await new Response(process.stdout).text()).split("\0")); |
There was a problem hiding this comment.
If git check-ignore takes longer than 1.5 seconds, the timeout kills it, but this code treats its partial output as complete. Ignored entries missing from that output are marked as not ignored and appear in the browser despite the default exclusion policy. Check that the query completed before using its results.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/hunk/src/app/documents/filesystemSource.ts
Line: 249
Comment:
**Incomplete Ignore Results Expose Entries**
If `git check-ignore` takes longer than 1.5 seconds, the timeout kills it, but this code treats its partial output as complete. Ignored entries missing from that output are marked as not ignored and appear in the browser despite the default exclusion policy. Check that the query completed before using its results.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 251ad80: ignore results are used only after the process exits normally with status 0 or 1. Timeout, cancellation, and error exits make the directory unavailable rather than exposing entries based on partial output. Added regression coverage for timeout with partial output and an error exit, and normalized Git output paths for Windows.
This comment was generated by Pi using GPT-6.1
| detail: "This directory is unavailable; symlinks are not followed.", | ||
| }; | ||
| const entries: DocumentEntry[] = []; | ||
| for await (const child of await opendir(key)) { |
There was a problem hiding this comment.
If another process can replace entries in the browsed directory, it can swap a checked directory for a symlink before opendir uses the path. The browser can then list names outside the chosen collection. The same check-then-use gap affects editor authorization: replacing a checked regular file before $EDITOR opens its path can redirect editing outside the collection. These operations need to preserve the identity of the entry they checked.
How this was verified: The listing opens a path after its symlink check, and the editor receives a path string after a separate regular-file check.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/hunk/src/app/documents/filesystemSource.ts
Line: 204
Comment:
**Path Checks Can Be Bypassed**
If another process can replace entries in the browsed directory, it can swap a checked directory for a symlink before `opendir` uses the path. The browser can then list names outside the chosen collection. The same check-then-use gap affects editor authorization: replacing a checked regular file before `$EDITOR` opens its path can redirect editing outside the collection. These operations need to preserve the identity of the entry they checked.
**How this was verified:** The listing opens a path after its symlink check, and the editor receives a path string after a separate regular-file check.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Addressed in 251ad80. Linux reads/enumeration now walk no-follow ancestors through retained directory handles and enumerate the checked descriptor via /proc/self/fd. Editors never receive the collection path: they edit a private copy, and writeback uses the retained original handle after identity/content conflict checks. Conflicts retain the edited copy for recovery. Regression tests cover directory swap-and-restore, file replacement, symlink redirection, and the real PTY editor workflow.
Non-Linux directory enumeration still uses portable Node path APIs with ancestry/identity revalidation; this is hardening, not a sandbox against an adversary repeatedly swapping and restoring ancestors. That limitation is explicit in docs/open.md rather than claiming the portable checks are race-proof.
This comment was generated by Pi using GPT-6.1
|
|
||
| /** Prevent metadata inspection from fetching missing objects or launching transport helpers. */ | ||
| function gitQueryEnvironment() { | ||
| return { ...process.env, GIT_NO_LAZY_FETCH: "1", GIT_ALLOW_PROTOCOL: "" }; |
There was a problem hiding this comment.
gitQueryEnvironment spreads process.env directly, and the new document bootstrap also defaults its environment parameter to process.env. The repository guideline says never to access process.env directly and to pass environment values from the application boundary. This requirement needs to be satisfied before merging.
Context Used: guidelines.mdc Cursor rule (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/hunk/src/app/documents/filesystemSource.ts
Line: 19
Comment:
**Direct Environment Access Violates Requirement**
`gitQueryEnvironment` spreads `process.env` directly, and the new document bootstrap also defaults its environment parameter to `process.env`. The repository guideline says never to access `process.env` directly and to pass environment values from the application boundary. This requirement needs to be satisfied before merging.
**Context Used:** guidelines.mdc Cursor rule ([source](https://github.com/modem-dev/modem/blob/main/.cursor/rules/guidelines.mdc))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Not applying this suggestion: the cited rule belongs to modem-dev/modem, not this repository. Hunk has no blanket ban on process.env; its existing filesystem/process and editor boundaries use it. Please scope guideline findings to Hunk’s own AGENTS.md and CONTRIBUTING.md.
This comment was generated by Pi using GPT-6.1
There was a problem hiding this comment.
You're right. Hunk's AGENTS.md and CONTRIBUTING.md do not prohibit direct process.env access, and the repository already uses it at application/process boundaries. My comment incorrectly carried over a guideline from another repository, so it is not an actionable finding for this PR. I'll scope future guideline findings to Hunk's own checked-in instructions.
|
Audited PR #1148 with Oxlint’s
Git metadata, bounded byte I/O, private-copy editing, navigation, keyboard routing, and content/status rendering now have focused modules. The older high-complexity startup/review functions remain outside this refactor; the audit did not treat them as new document code. Added coverage for physical-row navigation, live tree selection, menu parity, overlay priority, status parsing, and bounded/partial byte I/O. Typecheck, lint, formatting, dependency boundaries, full unit tests, TTY smoke, and all 194 PTY tests pass. GitHub CI also passes, including Windows compatibility, Linux validation/packaging, PTY integration, and macOS/Windows compiled portability. This comment was generated by Pi using GPT-6.1 |
This branch has not been deployed
1 participant
Summary
Add
hunk open [path]for reading complete syntax-highlighted files and browsing directories without a repository or changeset. The default path is..The read-only browser supports lazy tree expansion, keyboard/mouse navigation, wrapping and horizontal scrolling, line numbers, themes, remappable commands, live refresh, hidden/ignored visibility, lightweight Git status, and opening the displayed file in
$EDITOR.Approach
DocumentSourcewith opaque entry identities, explicit availability results, lazy listing/reading, observation, and optional editor capabilities. Filesystem and Git I/O stay in the application tier.HunkSessionHost, sharing renderer lifecycle, theme ownership, chrome, semantic keymaps and command construction.DiffFileor hunks and does not register a review session.Why a host feature rather than an extension?
Existing extension pane navigation and file views operate on files already represented in a review changeset. They cannot express an independent lazy collection of unchanged filesystem documents without inventing review data. This introduces the internal document-source seam and exercises it with a real host consumer; a public document extension API remains a separate, capability-based follow-up.
Safety and limits
itoggle.file:lineaddressing are not included in this MVP.Validation
Ran on Linux x64; macOS and Windows were not tested on actual machines.
bun run typecheckbun run lintbun run format:checkbun run deps:checkbun run test— both shards pass, zero failuresbun run test:integration— 193 pass, 1 platform skip, zero failures on two full rerunsbun run test:tty-smoke— 10 passbun test test/review-conformance— 118 passbun test packages/hunk/src/app/documents packages/hunk/src/ui/documents test/pty/open-integration.test.ts— 21 passbun run build:npm && bun run check:pack— packed facade/consumer checks pass, including 37 documented extension examplesbun run website:checkandbun run check:docsbun run install:bin, followed by real-PTY checks of the compiled document browser and a split diff reviewgit diff --checkThe initial full integration run hit two navigation timeouts; both passed in the dedicated navigation rerun and two subsequent full integration runs.
bun run knipreports the same six unused files, one dependency and two exports as clean upstreama3321c82; reproduced against a temporary archive of that revision, with no new findings.Coverage includes opaque identities, lazy demand, stale reads, observer teardown, ignored/tracked/hidden entries, binary/invalid UTF-8/large/missing/unreadable/symlink cases, atomic replacement, metadata hook/filter safety (including nested repositories), native wrapping, resizing, explicit keybinding unbindings, and keyboard/mouse navigation.
Real terminal evidence
Captured from the compiled binary in a real PTY using tuistory/Ghostty: Linux x64, 100 columns × 20 rows,
github-dark-default, directory tree and complete-document layout. The sanitized fixture is not a repository. Syntax color is additionally asserted in the PTY tests.Command:
hunk open <fixture-directory> --theme github-dark-default. Keyboard expandssrcand opensexample.ts; the PTY suite also clicks files/menus and exercises watch refresh and deletion.This is a text dump of the captured terminal frame, not redirected CLI stdout or a mockup. A PNG was also captured locally; native GitHub media attachment remains to be uploaded manually.
This PR description was generated by Pi using gpt-6.1-sol