[codex] Add experimental file access hooks for QA POC#249
Conversation
|
@codex review Please review this as an experimental change specifically for the agentic QA environment POC. Focus on correctness and safety of the passive file-access hook API, metadata collection, command attribution for dynamic accesses, and whether the instrumentation can accidentally affect rshell behavior. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9f02630df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if name := simpleCommandName(cm); name != "" { | ||
| r.fileAccessCommand = name |
There was a problem hiding this comment.
When the command name is produced by expansion (for example cmd=echo; $cmd *.txt or cmd=echo; $cmd $(<input.txt)), simpleCommandName returns empty because the first word is not purely literal, so fileAccessCommand stays empty while r.fields(args...) performs glob and command-substitution file accesses. The command is resolved from the expanded fields immediately afterward, but the hook events have already been emitted without attribution, which breaks the QA POC for dynamic shell commands.
Useful? React with 👍 / 👎.
| redirCommand := "" | ||
| if st.Cmd != nil { | ||
| redirCommand = simpleCommandName(st.Cmd) | ||
| } |
There was a problem hiding this comment.
For input redirects on commands whose name is expanded (for example cmd=cat; $cmd < input.txt), this pre-expansion simpleCommandName call returns "", so the input_redirect open event is emitted without the command even though the command later resolves to cat and runs successfully. This leaves a common dynamic-access path unattributed in the passive hook stream.
Useful? React with 👍 / 👎.
|
@codex review I addressed the first-pass findings by adding side-effect-free command attribution for simple expanded command names, covering glob expansion, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d96d1eb38
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for _, rd := range st.Redirs { | ||
| cls, err := r.redir(ctx, rd) | ||
| cls, err := r.redir(ctx, rd, redirCommand) |
There was a problem hiding this comment.
When a redirect word itself performs the $(<file) shortcut, r.redir expands it via r.literal(rd.Word) before opening the final redirect target, but this loop only passes redirCommand into the final r.open and never sets r.fileAccessCommand during the redirect-word expansion. In a case like cmd=cat; $cmd < $(<namefile), the read of namefile is emitted as a command_substitution access with an empty or stale Command, while the later input redirect is attributed to cat, which leaves the hook data inconsistent for exactly the passive attribution path this change adds.
Useful? React with 👍 / 👎.
| value := r.lookupVar(part.Param.Value).String() | ||
| if splitAndGlob && unsafeUnquotedCommandValue(value) { | ||
| return "", false |
There was a problem hiding this comment.
The side-effect-free attribution for unquoted parameter-expanded command names only rejects whitespace and glob metacharacters, but field splitting uses the current mutable IFS, not just space/tab/newline. With IFS=/; cmd=bin/echo; $cmd *.txt, the actual command field is split to bin with echo as an argument, while glob file-access events are attributed to bin/echo; any non-default IFS character inside $cmd can therefore corrupt the hook's command attribution. Consider treating unquoted parameter values containing any active IFS rune as unsafe, or deriving the name from the same field-splitting rules used by execution.
Useful? React with 👍 / 👎.
|
@codex review I addressed the second-pass findings by setting the file-access command hint while redirect words/heredocs are expanded, adding coverage for |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d53c26b9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| r.lastExpandExit = exitStatus{} | ||
| restoreAccessCommand := r.pushFileAccessCommand(r.fileAccessCommandName(cm)) | ||
| fields := r.fields(args...) | ||
| restoreAccessCommand() |
There was a problem hiding this comment.
When a simple command has an inline assignment whose value reads a file, e.g. TOKEN=$(<tokenfile) cat input, this restore clears fileAccessCommand before the later assignVal loop expands the assignment via r.literal. The $(<tokenfile) hook then fires with an empty/stale Command instead of cat, so the new file-access instrumentation loses attribution for env-assignment reads that are part of the command invocation; keep the hint active until after those assignments are expanded.
Useful? React with 👍 / 👎.
|
@codex review I addressed the third-pass finding by keeping the file-access command hint active through inline assignment expansion, with coverage for |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
This is an experimental change for the agentic QA environment POC.
It adds an opt-in
WithFileAccessHooksrunner option that lets library callers install passive before/after callbacks around rshell file access. The hook events carry command/source/operation/path context plus lightweight pre/post metadata so the QA shim and command proxy can capture file usage from rshell itself rather than duplicating shell parsing or command-specific path logic.Covered access surfaces include builtin file operations, input redirects, glob directory reads, and the
$(<file)command-substitution shortcut. The hooks are observability-only: they cannot authorize, deny, rewrite, or otherwise change file access behavior.Validation
make fmtgo test ./interp -run TestFileAccessHooks -count=1go test ./analysis -run 'Test.*AllowedSymbols|Test.*Verification' -count=1go test ./...Notes
This is intentionally scoped as POC instrumentation for the agentic QA workflow. It is not a new authorization or sandbox policy layer.