Repository navigation
fix(ce-doc-review): tell the peer the document is embedded so it never reads from disk - #1857
Conversation
…r reads from disk Refs EveryInc#1856
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 2b840ceb4c
ℹ️ 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".
| # The whole document is embedded below. Say so, so a peer never goes looking for the | ||
| # prompt or the document on disk: a read outside its workspace is a permission | ||
| # request that is denied, and a peer that stops to explain the denial returns no JSON. | ||
| printf 'The complete document is embedded below under "Document content". Do not call tools or read files; if any read is denied, ignore it and answer from the embedded text.\n' |
There was a problem hiding this comment.
When this route assembles the prompt, line 621 first injects the full <output-contract> from references/subagent-template.md, whose line 78 explicitly says the reviewer may use non-mutating tools including file reads. This new unconditional prohibition leaves both same-level instructions in every peer prompt, so a peer gathering feasibility context can still follow the read permission and reproduce the OpenCode external_directory denial/no-artifact outcome this change is intended to prevent. Reconcile the route-specific contract rather than appending a denied-read case so the assembled prompt contains one unambiguous tool-use rule.
AGENTS.md reference: AGENTS.md:L114-L114
Useful? React with 👍 / 👎.
| expect(sent).toContain("Do not call tools or read files") | ||
| expect(sent.indexOf("Do not call tools or read files")).toBeLessThan(sent.indexOf("Document content:")) | ||
| expect(sent).toContain("# embedded body") | ||
| expect(sent).not.toContain(path.dirname(sb.logs.prompt)) |
There was a problem hiding this comment.
sb.logs.prompt is the stub's copy path under /tmp/xmodel-doc-route-*, while the document created by makeDoc and the real --file prompt live in different temp trees (xmodel-doc-* and xmodel-doc-scratch-*). Consequently, leaking either relevant absolute path into sent—the regression this assertion claims to prevent—still leaves this check green. Retain the document path or read the --file argument from argv(sb) and assert against that actual directory so the new guard is falsifiable.
AGENTS.md reference: AGENTS.md:L181-L181
Useful? React with 👍 / 👎.
2 participants
Closes #1856
Summary
On the OpenCode route, the worker runs the peer in an empty temp workspace with
acpx --deny-all, and OpenCode'sexternal_directorypermission defaults toask. If the peer model goes looking for the prompt or the document on disk (an absolute path outside its workspace), OpenCode raises a permission request, acpx denies it, and the peer can end its turn with an explanation instead of the findings JSON, so the run publishes no artifact.The document is already embedded in the prompt, so no read is ever needed. This adds one sentence to the peer prompt, ahead of the schema: the complete document is embedded below, do not call tools or read files, and ignore a denied read. No permission or isolation setting changes. The empty workspace stays empty, and no path is added to the OpenCode permission block. The code-review worker is untouched: it already allows
external_directorybecause it stages large diffs outside the repo on purpose.Validation
bun test tests/skills/ce-doc-review-cross-model-routes.test.ts tests/skills/ce-code-review-cross-model-routes.test.ts).session/request_permissionforexternal_directory, acpx exits 5). Ordinary runs did not trigger it, so the failure is model-dependent and I could not show a before/after rate.Security Disclosure
No security-relevant changes. Prompt text only; permissions, workspace isolation, and egress are unchanged.
Agent Disclosure
Claude Code · claude-sonnet-5-5