Uh oh!
There was an error while loading. Please reload this page.
feat(agent): resolve review feedback with evidence - #16
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
Uh oh!
There was an error while loading. Please reload this page.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
Implements the v0.1 feedback-to-verification loop: ingest GitHub review feedback, validate whether the claim is actually supported, apply a narrowly scoped fix through the runner boundary when authorized, verify with the repository's own checks, and publish deterministic evidence for accepted and rejected findings alike. Verdicts and evidence - Add a runtime verdict (accepted, rejected, inconclusive) decided by packages/agent/src/validation.ts rather than by the reviewer or the model. A finding is accepted only when it cites evidence, names a file that exists, and quotes repository content that is really there, so fabricated paths and invented code quotes are rejected instead of fixed. - Preserve rejection reasons in full and render an EvidenceBundle to deterministic Markdown, shared by the CLI and GitHub checks. - Derive TaskResult.verified in exactly one place: completed state, an applied change, and every executed check passing. No branch can claim verification it did not earn. Lifecycle - Add LifecycleMachine with the full transition table, so executing cannot reach completed without verifying and an undefined move throws rather than producing a result that looks finished. - Enforce the repair budget, narrow-scope change limits, and a distinct reportable outcome for every authorization refusal. Runner boundary - Split the boundary into process, boundary, local, and container modules. Refuse writes on a read-only runner, refuse anything reaching into .git, and re-validate paths against resolved symlinks so a planted link cannot escape the checkout. - Add ContainerRunner for production: ephemeral, all capabilities dropped, no-new-privileges, CPU and memory limits, no forwarded environment, and the configured egress policy mapped to a concrete container network. - Bound and redact captured output; reject commands that would need a shell. GitHub - Parse pull_request_review and pull_request_review_comment into normalized feedback, validating every field of the untrusted payload and ignoring approvals, empty bodies, and the agent's own account. - Publish check runs whose conclusion can never report a failing check as success, with the token sent only as a header and redacted from errors. Verification and configuration - Discover the checkout's own lint, typecheck, test, and build scripts when checks is empty, and refuse to change files when no check can verify them. - Add validation, runner, and maxChangedFiles policy with full validation. - Require callers to pass a GitHub token explicitly so no code path reaches GitHub implicitly, and keep every test off the network. Verified with pnpm check:repo, lint:ci, typecheck, test (170 tests), and build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBHWebMnjZkofzCY7Biawm
Validation alone raced the filesystem operation: a concurrent task could swap a validated directory for a symlink before mkdir or writeFile used the unresolved target, redirecting the write outside the checkout. Reads and writes now re-resolve the parent immediately before opening, open the final component with O_NOFOLLOW, and re-check containment on the descriptor itself (via /proc on Linux, inode identity elsewhere) before any content moves through it. Missing directories are created one validated component at a time instead of with a recursive mkdir. Deterministic regression tests reproduce the validate-then-swap race for both read and write.
Doctor's check discovery read lockfiles and package.json with direct filesystem helpers, splitting checkout access policy across packages. It now inspects the checkout through a read-only LocalRunner, so the same path validation and boundary enforcement apply. A local runner is used deliberately so doctor can still diagnose a misconfigured container isolation setup.
A write previously truncated and wrote through a descriptor whose containment was proven only at open time; a concurrent task could rename the opened inode outside the checkout in between and receive the write. Writes no longer mutate the target inode at all. Content is staged into a fresh exclusively-created temporary inode and committed with an atomic rename, with both names resolved through a held, containment-verified directory descriptor (/proc/self/fd on Linux), so the mutation stays anchored to the validated directory inode for its full lifetime. The final-component symlink refusal is kept explicit and the previous file mode is carried over the rename. A deterministic regression test renames the validated target over an external victim after every check has passed and proves the victim receives nothing.
ShellAnalyzeResult exposes commands, not commandNames, so lint and typecheck failed. assertSimpleCommand now throws CommandRejectedError, preserving the boundary's rejection contract; the class moves to process.ts where the lower layer can raise it.
The config layer intentionally validates only that a check command is non-empty; ViteHub Shell analysis in the runner rejects shell expressions at execution time. Update the stale tests to that contract.
… API error bodies Distinguish unparseable JSON from schema mismatches, append the redacted response body to API call failures, enable structured outputs on the provider, and give mocked chat completions a finish_reason so AI SDK parses their output.
aube's no-downgrade trust policy refuses @vite-hub/shell@0.0.2 because it carries no trust evidence while 0.0.1 was published with SLSA provenance. 0.0.3 restores provenance and keeps the same analyze API, so the boundary code is unchanged. Regenerates the lockfile for the rebased manifests.
Main introduced @e18e/eslint-plugin. Hoist per-call regular expressions to module scope and build the task list with Array.from's map callback.
The command-policy refactor made replaceInside private while the descriptor race regression test still overrides it to interleave a rename. Keep the method protected, matching its documented test seam, and align the override with the new parameter names.
0a4034a to
24f8312CompareUh oh!
There was an error while loading. Please reload this page.
| const real = await realpath(fallback); | ||
| const [expected, current] = await Promise.all([directory.stat(), stat(real)]); | ||
| if (expected.dev !== current.dev || expected.ino !== current.ino) | ||
| throw new PathEscapeError(original, 'parent replaced while writing'); | ||
| return real; |
There was a problem hiding this comment.
When /proc/self/fd is unavailable, directoryAnchor verifies that fallback names the held directory and then returns that mutable pathname. A concurrent task can rename the verified directory and replace it with a symlink immediately after the inode comparison. replaceInside subsequently creates its temporary file and renames the result through that symlink, allowing a repository-relative write to overwrite a file outside the checkout. Use descriptor-relative create/rename operations for this path, or fail closed when those operations are unavailable.
Artifacts
Forced fallback race reproduction script
- Node script that forces the current directory-anchor fallback and interleaves the directory-to-symlink swap after identity verification, demonstrating the tested condition.
Control run with forced fallback and no directory swap
- Executed control command shows forced fallback writes the target inside the checkout and does not create an outside file, establishing the comparison baseline.
Adversarial forced-fallback run after directory swap
- Executed adversarial command shows the post-verification symlink swap causes the target write to appear outside the checkout, reproducing the candidate.
Runtime harness for the two prior boundary claims
- Node harness implements the current boundary write ordering and deterministically interleaves each previously reported race condition.
Runtime classification of prior boundary claims
- Executed checks show the symlink validation race is rejected without an outside file and the renamed target inode retains old content while the checkout receives the payload, so both prior roots are no longer present.
Unavailable runner test command output
- Captured attempted Vitest command exits 127 because the repository has no node_modules test binary, explaining why the narrow Node runtime harness was used.
Unavailable runner typecheck command output
- Captured attempted TypeScript command exits 127 because the repository has no node_modules compiler binary, confirming the environment limitation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/runner/src/boundary.ts
Line: 375-379
Comment:
**Fallback directory anchor can be redirected outside the checkout**
When `/proc/self/fd` is unavailable, `directoryAnchor` verifies that `fallback` names the held directory and then returns that mutable pathname. A concurrent task can rename the verified directory and replace it with a symlink immediately after the inode comparison. `replaceInside` subsequently creates its temporary file and renames the result through that symlink, allowing a repository-relative write to overwrite a file outside the checkout. Use descriptor-relative create/rename operations for this path, or fail closed when those operations are unavailable.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Implements the v0.1 feedback-to-verification loop: ingest GitHub review
feedback, validate whether the claim is actually supported, apply a narrowly
scoped fix through the runner boundary when authorized, verify with the
repository's own checks, and publish deterministic evidence for accepted and
rejected findings alike.
Verdicts and evidence
packages/agent/src/validation.ts rather than by the reviewer or the model. A
finding is accepted only when it cites evidence, names a file that exists,
and quotes repository content that is really there, so fabricated paths and
invented code quotes are rejected instead of fixed.
deterministic Markdown, shared by the CLI and GitHub checks.
change, and every executed check passing. No branch can claim verification it
did not earn.
Lifecycle
reach completed without verifying and an undefined move throws rather than
producing a result that looks finished.
reportable outcome for every authorization refusal.
Runner boundary
Refuse writes on a read-only runner, refuse anything reaching into .git, and
re-validate paths against resolved symlinks so a planted link cannot escape
the checkout.
no-new-privileges, CPU and memory limits, no forwarded environment, and the
configured egress policy mapped to a concrete container network.
GitHub
feedback, validating every field of the untrusted payload and ignoring
approvals, empty bodies, and the agent's own account.
success, with the token sent only as a header and redacted from errors.
Verification and configuration
checks is empty, and refuse to change files when no check can verify them.
GitHub implicitly, and keep every test off the network.
Verified with pnpm check:repo, lint:ci, typecheck, test (170 tests), and build.
Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01LBHWebMnjZkofzCY7Biawm
Need help on this PR? Tag
@codesmithwith what you need. Autofix is enabled.Confidence Score: 3/5
The change is not safe to merge until the non-
/proc/self/fdwrite path preserves checkout containment for the entire filesystem mutation.One verified security-sensitive checkout escape remains: the fallback turns a held directory capability back into a mutable pathname before creating and renaming the replacement file.
Files Needing Attention: packages/runner/src/boundary.ts
Security Review
On platforms without usable
/proc/self/fd, an actor able to modify the writable checkout can replace a verified parent directory with a symlink after its identity check. Temporary-file creation and the final rename then follow the replacement path, allowing a repository-relative write to create or overwrite a file outside the checkout.What T-Rex did
Prompt To Fix All With AI
Reviews (4): Last reviewed commit: "fix(runner): keep the adversarial-rename..." | Re-trigger Greptile