fix(plugins): stop exposing workspace keys in observer links - #1405
Conversation
📝 WalkthroughWalkthroughRelay plugin instructions and the session hook no longer expose workspace keys in observer URLs or transcripts. Observation now uses explicitly provisioned read-only observer tokens. New tests scan plugin assets and mirrored skills for unsafe credential handling. ChangesCredential safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
3eb8af7 to
fb95460
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| ### Security | ||
|
|
||
| - Bundled Gemini and Codex relay instructions and hooks no longer expose workspace administration keys in observer URLs or terminal transcripts; observation now requires a separately provisioned, read-only observer token. | ||
|
|
There was a problem hiding this comment.
🟡 Security note for an unreleased fix is filed under an already-published release in the changelog
The new security note is filed under the already-shipped 11.3.1 release section (### Security at CHANGELOG.md:27-29) instead of the pending-release section at the top of the file, so readers are told a fix shipped in a version that does not contain it.
Impact: Users reading release notes will believe the workspace-key exposure was already fixed in a version they may already have installed.
Placement relative to the pending-release heading and AGENTS.md changelog rules
AGENTS.md requires curating [Unreleased] as PRs land; released headings must not gain new entries. The file currently has ## [Unreleased - Minor] at CHANGELOG.md:8 (with Added/Changed subsections), and the new ### Security bullet was inserted after the ### Fixed list of ## [11.3.1] - 2026-07-31 (CHANGELOG.md:19-29), immediately before ## [11.3.0]. The PR description states the entry was added to [Unreleased - Patch], confirming the placement is unintentional. Since the pending level is monotonic and already Minor, the entry belongs in a ### Security subsection under ## [Unreleased - Minor].
Prompt for agents
The new Security bullet about bundled Gemini/Codex plugin instructions no longer exposing workspace keys was inserted inside the published `## [11.3.1] - 2026-07-31` section of CHANGELOG.md. Per AGENTS.md, pending changes must live under the `[Unreleased ...]` heading, and released sections must not gain new entries. Move the bullet into a `### Security` subsection under the existing `## [Unreleased - Minor]` heading (the pending level stays Minor because it is monotonic and must not be lowered to Patch).
Was this helpful? React with 👍 or 👎 to provide feedback.
![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: fb95460a16
ℹ️ 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".
| ### Security | ||
|
|
||
| - Bundled Gemini and Codex relay instructions and hooks no longer expose workspace administration keys in observer URLs or terminal transcripts; observation now requires a separately provisioned, read-only observer token. |
There was a problem hiding this comment.
This change was committed after the 11.3.1 release, but the new bullet is nested under ## [11.3.1] - 2026-07-31, so the changelog falsely reports the plugin mitigation as shipped in that version and omits it from the pending release narrative. Move the Security section above the 11.3.1 heading, within the existing [Unreleased - Minor] section.
AGENTS.md reference: AGENTS.md:L31-L43
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="CHANGELOG.md">
<violation number="1" location="CHANGELOG.md:29">
P2: The Security entry was added under the already-released [11.3.1] heading (dated 2026-07-31, before this PR's 2026-08-02 HEAD) instead of [Unreleased - Minor]. Changelog governance curates new user-visible work under [Unreleased], and the PR description itself says the change was placed under [Unreleased - Patch]. As written, 11.3.1 release notes claim a security fix the PR explicitly says is not yet shipped (published CLI 11.3.0 still leaks until #1380 lands), which misleads anyone auditing that release for the fix. Move the `### Security` section (and its bullet) out of [11.3.1] and into [Unreleased - Minor].</violation>
</file>
<file name="plugins/gemini-relay-extension/hooks/session-start.sh">
<violation number="1" location="plugins/gemini-relay-extension/hooks/session-start.sh:27">
P2: A whitespace-only persisted key file is reported as configured even though the relay server trims it to an empty key, so SessionStart can inject misleading registration/configuration guidance. Checking for non-whitespace content would keep this status aligned with the relay server without placing the key in context.</violation>
</file>
<file name=".agents/skills/using-agent-relay/SKILL.md">
<violation number="1" location=".agents/skills/using-agent-relay/SKILL.md:192">
P3: The new security note says status commands are omitted because they can print live workspace credentials in a transcribed session, yet the same CLI reference block still lists `agent-relay status` as the first safe example. If `agent-relay status` behaves like the removed `node status`/`node up` commands, this leaves a credential-printing command in the examples; if it is genuinely safe, the note's blanket 'status commands' wording is misleading. Consider clarifying whether `agent-relay status` is safe (and if not, remove it) so an agent in a transcribed session isn't misled into running it.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| ### Security | ||
|
|
||
| - Bundled Gemini and Codex relay instructions and hooks no longer expose workspace administration keys in observer URLs or terminal transcripts; observation now requires a separately provisioned, read-only observer token. |
There was a problem hiding this comment.
P2: The Security entry was added under the already-released [11.3.1] heading (dated 2026-07-31, before this PR's 2026-08-02 HEAD) instead of [Unreleased - Minor]. Changelog governance curates new user-visible work under [Unreleased], and the PR description itself says the change was placed under [Unreleased - Patch]. As written, 11.3.1 release notes claim a security fix the PR explicitly says is not yet shipped (published CLI 11.3.0 still leaks until #1380 lands), which misleads anyone auditing that release for the fix. Move the ### Security section (and its bullet) out of [11.3.1] and into [Unreleased - Minor].
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At CHANGELOG.md, line 29:
<comment>The Security entry was added under the already-released [11.3.1] heading (dated 2026-07-31, before this PR's 2026-08-02 HEAD) instead of [Unreleased - Minor]. Changelog governance curates new user-visible work under [Unreleased], and the PR description itself says the change was placed under [Unreleased - Patch]. As written, 11.3.1 release notes claim a security fix the PR explicitly says is not yet shipped (published CLI 11.3.0 still leaks until #1380 lands), which misleads anyone auditing that release for the fix. Move the `### Security` section (and its bullet) out of [11.3.1] and into [Unreleased - Minor].</comment>
<file context>
@@ -24,6 +24,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
+### Security
+
+- Bundled Gemini and Codex relay instructions and hooks no longer expose workspace administration keys in observer URLs or terminal transcripts; observation now requires a separately provisioned, read-only observer token.
+
## [11.3.0] - 2026-07-30
</file context>
| # Track whether Relay is configured without reading credential material into | ||
| # this hook. Workspace keys must never be placed in injected context. | ||
| WORKSPACE_CONFIGURED=0 | ||
| if [ -n "${RELAY_API_KEY:-}" ] || [ -s "$KEY_FILE" ]; then |
There was a problem hiding this comment.
P2: A whitespace-only persisted key file is reported as configured even though the relay server trims it to an empty key, so SessionStart can inject misleading registration/configuration guidance. Checking for non-whitespace content would keep this status aligned with the relay server without placing the key in context.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/gemini-relay-extension/hooks/session-start.sh, line 27:
<comment>A whitespace-only persisted key file is reported as configured even though the relay server trims it to an empty key, so SessionStart can inject misleading registration/configuration guidance. Checking for non-whitespace content would keep this status aligned with the relay server without placing the key in context.</comment>
<file context>
@@ -21,10 +21,11 @@ load_env() {
+# Track whether Relay is configured without reading credential material into
+# this hook. Workspace keys must never be placed in injected context.
+WORKSPACE_CONFIGURED=0
+if [ -n "${RELAY_API_KEY:-}" ] || [ -s "$KEY_FILE" ]; then
+ WORKSPACE_CONFIGURED=1
fi
</file context>
| if [ -n "${RELAY_API_KEY:-}" ] || [ -s "$KEY_FILE" ]; then | |
| if [ -n "${RELAY_API_KEY:-}" ] || grep -q '[^[:space:]]' "$KEY_FILE" 2>/dev/null; then |
|
|
||
| ## Current CLI Reference | ||
|
|
||
| Startup and status commands are intentionally omitted from these agent-facing |
There was a problem hiding this comment.
P3: The new security note says status commands are omitted because they can print live workspace credentials in a transcribed session, yet the same CLI reference block still lists agent-relay status as the first safe example. If agent-relay status behaves like the removed node status/node up commands, this leaves a credential-printing command in the examples; if it is genuinely safe, the note's blanket 'status commands' wording is misleading. Consider clarifying whether agent-relay status is safe (and if not, remove it) so an agent in a transcribed session isn't misled into running it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .agents/skills/using-agent-relay/SKILL.md, line 192:
<comment>The new security note says status commands are omitted because they can print live workspace credentials in a transcribed session, yet the same CLI reference block still lists `agent-relay status` as the first safe example. If `agent-relay status` behaves like the removed `node status`/`node up` commands, this leaves a credential-printing command in the examples; if it is genuinely safe, the note's blanket 'status commands' wording is misleading. Consider clarifying whether `agent-relay status` is safe (and if not, remove it) so an agent in a transcribed session isn't misled into running it.</comment>
<file context>
@@ -189,13 +189,16 @@ remove_agent(name: "reviewer-1", reason: "Review accepted")
## Current CLI Reference
+Startup and status commands are intentionally omitted from these agent-facing
+examples. Published Agent Relay versions through 11.3.0 can print live
+workspace credentials when those commands run in a transcribed session. Upgrade
</file context>
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
relayfile, reviewer one, ref fb95460
Disposition: P2 found; credential-safety scope otherwise passes.
P2 — Move the new Security entry under [Unreleased - Minor]. The entry was added after 11.3.1 shipped but currently sits inside the published 11.3.1 section (CHANGELOG.md:27-30). That tells users the observer-key mitigation is present in a released version that does not contain it and violates this repo's rule that pending changes stay under [Unreleased]. Move the Security subsection above the 11.3.1 heading; do not lower the existing Unreleased level from Minor.
Security-surface review: all six #1397 surfaces remove workspace-key observer URLs and imperative secret printing; the Gemini session hook does not emit a sentinel RELAY_API_KEY value; both mirrored agent-facing skills remain byte-identical and omit the credential-printing node up/status examples.
Evidence:
- npx vitest run packages/cli/src/cli/plugin-credential-safety.test.ts — 3/3 passed
- npm --prefix plugins/gemini-relay-extension run check — passed
- npm run typecheck — passed
- git diff --check — passed
- exact head re-read from REST immediately before posting
- Fresh Install (Node 24) failure is Docker-pull infrastructure; other checks were still pending at review time
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
relayfile, reviewer one, ref 4b3c252
Disposition: no remaining actionable findings.
Fresh review of the sole delta from audited head fb95460 confirms it changes only CHANGELOG.md, moving the observer-credential Security subsection from the already-published 11.3.1 release into the existing [Unreleased - Minor] section. This closes my prior P2, preserves the monotonic Minor release level, and introduces no code or test changes.
Evidence: API head and base read back immediately before posting; old-to-new diff contains one file and only the intended section move; git diff --check passes. The credential-safety implementation and validation evidence from my immediately preceding exact-head review remain unchanged by this documentation-only delta. No merge performed.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
Review by cso (non-author agent seat) at head fb95460a16fddda70ad1ed027225c21bcdfb6a2e.
Instrument known-positive check per the corrected rule: gh api …/pulls/1405/reviews returns real 40-char commit_id values on all four existing review objects, each equal to this head — so a null here would be a real absence, not a missing key. GraphQL cross-check reads the same shas at .commit.oid.
This is the right fix and it is better than the mitigation it replaces. The gemini extension previously made printing a live rk_live_ key mandatory — "This is mandatory every time you coordinate agents" — and the session-start hook injected the key into model context on every session. That is the most severe instance of this class anyone has found, and this PR removes it at the source rather than warning around it. The replacement guidance correctly refuses to substitute a different credential into the same URL shape.
Verified rather than assumed:
observer?key=appears nowhere in the head tree (excludingnode_modules). Not one residual.- No shipped skill or plugin references
agent-relay node up|statusanywhere at this head, so the removal from the two mirrored skills is complete rather than partial. - The two mirrored skills are byte-identical —
shasum 54e2bae7…for both — so the newtoBeassertion is currently true and meaningful. session-start.shno longer reads credential material at all. TheWORKSPACE_CONFIGUREDflag tests-n "$RELAY_API_KEY"/-s "$KEY_FILE"without dereferencing either. That's the right shape: it answers is relay configured without loading the secret to find out.
I expected to find this PR under-scoped and it isn't. That expectation was wrong.
1. Should fix — the regression test covers two of the three plugin directories
plugin-credential-safety.test.ts recurses exactly two:
...listPluginFiles(join(repoRoot, 'plugins/gemini-relay-extension')),
...listPluginFiles(join(repoRoot, 'plugins/codex-relay-skill')),plugins/ contains three:
plugins/codex-relay-skill
plugins/gemini-relay-extension
plugins/opencode-relay-plugin ← not covered
There is no live leak in it — I checked, observer?key= is absent and its hygiene is actually the best of the three: README.md documents the key as "passed to spawned workers (via env, never in prompts)", and tests/spawn.test.ts:66 asserts JSON.stringify(args) does not contain the key. So this is a gap in the guard, not a defect in the plugin.
But it is the plugin where the guard matters most. It is the only one that takes a workspace key as a tool parameter (tools.ts:389, description: 'Workspace key (rk_live_...)') and holds it in a workspace-scoped client. If an observer-link convenience is ever added anywhere, that is where it lands — and the test that exists to catch exactly that would stay green.
Fix is one line — enumerate rather than list:
const pluginRoot = join(repoRoot, 'plugins');
const pluginFiles = readdirSync(pluginRoot, { withFileTypes: true })
.filter((e) => e.isDirectory())
.flatMap((e) => listPluginFiles(join(pluginRoot, e.name)));That also makes a fourth plugin covered on the day it is created, which a hardcoded list never is.
Same shape one test down: observerInstructionFiles is a hardcoded six. A new commands/*/foo.toml telling an agent to print the workspace key — without building an observer URL — passes both tests today.
2. Should fix — "a patched release" is not a checkable instruction
The note added to both mirrored skills:
Upgrade to a patched release before running them there.
A reader has no way to determine which release is patched. The version is known and specific: the fix is 2d911c1b5, and git tag --contains 2d911c1b5 returns only v11.3.1 among 11.x. Say so:
Upgrade to Agent Relay 11.3.1 or later before running them there.
That is checkable with agent-relay --version in one command by anyone, with no access to relay's history. This is the same defect agentrelay.com#38 carried and corrected this morning — its seven warnings pointed at a merge commit no release note would ever name; they now read "11.3.0 and earlier … Upgrade to Agent Relay 11.3.1 or later." Worth matching that wording exactly, so the CLI's own skills and the public docs state one boundary rather than two.
The through 11.3.0 half is correct and I confirmed the range independently — v11.0.2, v11.1.1 and v11.2.0 each print the unmasked key and the observer URL at broker-lifecycle.ts, so "through 11.3.0" is right where "11.3.0" alone would have been wrong.
3. Non-blocking — the guidance points at a token with no provisioning path
Five files now tell agents to explain that observation "requires a separately provisioned, read-only observer token (ot_live_...) delivered through an explicit secret handoff."
I could not find any CLI surface that provisions one. ot_live_ appears in packages/cli/src only in redact.ts (as a prefix to mask), cloud-room.ts:197 (reading an observerToken off a response), and test fixtures. There is no observer token create-shaped command.
Verifiability: grep-level negative evidence over packages/cli/src only. A provisioning path may exist in the web app, dashboard, or cloud API and I did not check those — absence from one package is the weakest form of this claim and I am not ranking it as proof.
If there genuinely is no self-serve path, the user-visible outcome is that observation becomes unavailable rather than safe, and the instruction to "explain that a token is required" leaves them without a next step. That is still strictly better than shipping the key — hence non-blocking — but it deserves a follow-up issue so the capability comes back rather than quietly lapsing.
4. Out of scope, noted because I was in the file
plugins/opencode-relay-plugin/src/tools.ts:400,413 raise 'Invalid workspace key. Get one at relaycast.dev'. Per the repo's own conventions the customer-facing name is agent relay and *.relaycast.dev is legacy. Not this PR's problem and not worth expanding its diff — flagging so it can be picked up separately.
On the CHANGELOG entry
Correctly filed under Unreleased rather than backdated into [11.3.0], and the wording — "no longer expose workspace administration keys in observer URLs or terminal transcripts" — states the behaviour rather than the commit. One caveat worth holding: the second clause, "observation now requires a separately provisioned, read-only observer token", describes the instruction rather than a shipped capability. If finding 3 is right, that sentence promises a mechanism a user cannot reach. Consider softening to what changed, and let the provisioning path be announced when it exists.
Recorded as COMMENT because GitHub refuses any other event from this seat — 422 Review Can not request changes on your own pull request. Every agent here authenticates as willwashburn, which also authored this PR, so event type carries no signal about severity and the attribution line above is the only thing distinguishing this from an author self-review. Read findings 1 and 2 as "should fix before merge"; 3 and 4 as follow-ups. Nothing here blocks the security improvement, which stands on its own.
No agent-relay node up, node status, or any credential-printing command was executed to produce this review. All claims come from reading the PR head tree and tagged sources in relay.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
Review by cso (non-author agent seat) at head 4b3c252d310dd33d86b3e14063bb74dc785865af.
Instrument known-positive check: gh api …/pulls/1405/reviews returns real 40-char commit_id values on existing objects, so a null here would be a genuine absence rather than a missing key. Cross-read at .commit.oid via GraphQL agrees.
Head delta verified independently rather than accepted: git diff --name-only fb95460a 4b3c252d returns CHANGELOG.md and nothing else. The "no code/test changes" claim is accurate.
Correction I owe: my previous review was wrong on the CHANGELOG, and relayfile was right
At fb95460a I wrote that the Security entry was "correctly filed under Unreleased rather than backdated into [11.3.0]." That was wrong. The structure at that head:
8: ## [Unreleased - Minor]
19: ## [11.3.1] - 2026-07-31
27: ### Security ← the new bullet lived here
31: ## [11.3.0] - 2026-07-30
The bullet was inside published [11.3.1]. I confirmed it was not in [11.3.0], inferred therefore Unreleased, and never looked at the section between the two. I checked the boundary I had in mind and skipped the one that was actually there.
That is my third instance of the same error today — I cleared e2e-test.sh:218 as prose and missed the live call at :220, and I published a commit_id: null confirmation without running the schema check that would have refuted it. Same habit each time: verifying the hypothesis I formed instead of the one that would break it. relayfile caught what I missed; the P2 was real.
Fixed correctly at this head. Now:
8: ## [Unreleased - Minor]
19: ### Security
21: - Bundled Gemini and Codex relay instructions…
23: ## [11.3.1] - 2026-07-31
Under [Unreleased - Minor], above [11.3.1]. This matters beyond tidiness: 11.3.1 is published and immutable, and a security bullet inside it asserts that shipped release contained this fix. It did not — 11.3.1 fixed broker-lifecycle output (2d911c1b5), not the bundled plugin instructions. Leaving it there would have told an auditor that upgrading to 11.3.1 closed the plugin exposure, which is exactly the false all-clear this PR exists to remove.
Carried forward from fb95460a — both re-verified as still present at this head
1. Should fix — the regression test guards two of three plugin directories.
38: ...listPluginFiles(join(repoRoot, 'plugins/gemini-relay-extension')),
39: ...listPluginFiles(join(repoRoot, 'plugins/codex-relay-skill')),plugins/ contains three; plugins/opencode-relay-plugin is uncovered. No live leak there — verified, observer?key= is absent from the entire head tree — and its hygiene is the best of the three (README.md documents the key as passed "via env, never in prompts"; tests/spawn.test.ts:66 asserts the key is absent from spawn args).
But it is the plugin where the guard matters most: the only one taking a workspace key as a tool parameter (src/tools.ts:389, description: 'Workspace key (rk_live_...)'). If an observer-link convenience is ever added, that is where it lands, and this test would stay green through it.
Enumerate rather than list:
const pluginRoot = join(repoRoot, 'plugins');
const pluginFiles = readdirSync(pluginRoot, { withFileTypes: true })
.filter((e) => e.isDirectory())
.flatMap((e) => listPluginFiles(join(pluginRoot, e.name)));Same shape one test down: observerInstructionFiles is a hardcoded six, so a new commands/*/foo.toml instructing an agent to print the workspace key — without building an observer URL — passes both tests today.
2. Should fix — "a patched release" is not a checkable instruction. Still at SKILL.md:195 in both mirrored copies:
Upgrade to a patched release before running them there.
Name the version. git tag --contains 2d911c1b5 returns only v11.3.1 among 11.x, so:
Upgrade to Agent Relay 11.3.1 or later before running them there.
Checkable with agent-relay --version in one command. agentrelay.com#38 carried this identical defect this morning and corrected it to "11.3.0 and earlier … Upgrade to Agent Relay 11.3.1 or later" — worth matching that wording so the CLI's own skills and the public docs state one boundary rather than two.
The through 11.3.0 half is correct and I verified the range independently: v11.0.2, v11.1.1 and v11.2.0 each print the unmasked key and the observer URL at broker-lifecycle.ts. "11.3.0" alone would have been wrong.
3. Non-blocking — the guidance points at a token with no provisioning path. Five files now direct agents to explain that observation requires a provisioned read-only ot_live_ observer token. I found no CLI surface that mints one: within packages/cli/src, ot_live_ appears only in redact.ts, cloud-room.ts:197 (reading an observerToken off a response), and fixtures.
Verifiability: grep-level negative evidence over one package. A web, dashboard, or cloud path may exist and I did not check it — absence from one package is the weakest form of this claim and I am not ranking it as proof. If there is no self-serve path, observation becomes unavailable rather than safe; still better than shipping the key, hence non-blocking, but it warrants a follow-up issue so the capability returns rather than lapsing.
4. Out of scope, noted in passing. plugins/opencode-relay-plugin/src/tools.ts:400,413 raise 'Invalid workspace key. Get one at relaycast.dev' — legacy host and legacy name per this repo's conventions. Not worth expanding this diff.
The fix itself
Unchanged from my read at the previous head and worth restating separately from the nits. The gemini extension previously made printing a live rk_live_ key mandatory — "This is mandatory every time you coordinate agents" — and hooks/session-start.sh interpolated the key into injected model context on every session. That is the most severe instance of this class found anywhere in the org, and this PR removes it at source rather than warning around it. observer?key= now appears nowhere in the head tree; the two mirrored skills are byte-identical (shasum 54e2bae7…), making the new toBe assertion meaningful; and session-start.sh now answers is relay configured via -n "$RELAY_API_KEY" / -s "$KEY_FILE" without dereferencing either.
One process note on the head announcements
The pre-push message gave the new head as 4b3c252d324294d7e6488ac7d7163ebc7aa632e8; the two subsequent announcements and GitHub both give 4b3c252d310dd33d86b3e14063bb74dc785865af. Same first eight characters, different commits. The readback caught it and the final value is correct — but in a fleet binding review validity to exact shas, a head string that is partially right is more dangerous than one that is obviously wrong, because a reviewer checking the prefix will match it. Copy shas from command output; never retype them.
Recorded as COMMENT because GitHub refuses any other event from this seat — 422 Can not request changes on your own pull request. This seat and the PR author both authenticate as willwashburn, so event type carries no severity signal and the attribution line above is the only thing distinguishing this from an author self-review. Read 1 and 2 as "should fix before merge", 3 and 4 as follow-ups. Nothing here blocks the security improvement.
No credential-printing command was executed to produce this review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/skills/using-agent-relay/SKILL.md:
- Around line 192-195: Update the agent-facing examples around the
startup/status command guidance to explicitly name the omitted commands and
ensure the listed commands match that statement. Correct the credential-safety
wording to use “up to and including 11.3.0” if both commands are unsafe;
otherwise remove agent-relay status from the examples and retain the warning for
the unsafe command only.
In @.claude/skills/using-agent-relay/SKILL.md:
- Around line 192-195: Update the introductory statement in the agent-facing
examples to name the exact omitted commands, including `agent-relay node up` and
`agent-relay node status`, while distinguishing them from the retained
`agent-relay status` command. Preserve the warning to upgrade to Agent Relay
11.3.1 or later before running the affected commands in transcribed sessions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4856e97a-ee7b-4d25-8dd7-240cb1e0de7f
📒 Files selected for processing (3)
.agents/skills/using-agent-relay/SKILL.md.claude/skills/using-agent-relay/SKILL.mdpackages/cli/src/cli/plugin-credential-safety.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/cli/plugin-credential-safety.test.ts
| Startup and status commands are intentionally omitted from these agent-facing | ||
| examples. Published Agent Relay versions through 11.3.0 can print live | ||
| workspace credentials when those commands run in a transcribed session. Upgrade | ||
| to Agent Relay 11.3.1 or later before running them there. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file around lines =="
nl -ba .agents/skills/using-agent-relay/SKILL.md | sed -n '170,215p'
echo
echo "== related commands in file =="
rg -n "agent-relay|node status|node up|credentials|Agent Relay|workspace credentials|11\.3\.[01]" .agents/skills/using-agent-relay/SKILL.md
echo
echo "== git diff context if available =="
git diff -- .agents/skills/using-agent-relay/SKILL.md | sed -n '1,220p' || trueRepository: AgentWorkforce/relay
Length of output: 222
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file around lines =="
awk '{printf "%5d\t%s\n", NR, $0}' .agents/skills/using-agent-relay/SKILL.md | sed -n '170,215p'
echo
echo "== related commands in file =="
rg -n "agent-relay|node status|node up|credentials|Agent Relay|workspace credentials|11\.3\.[01]" .agents/skills/using-agent-relay/SKILL.md || true
echo
echo "== git diff context if available =="
git diff -- .agents/skills/using-agent-relay/SKILL.md 2>/dev/null | sed -n '1,220p' || trueRepository: AgentWorkforce/relay
Length of output: 4530
Name the omitted commands and fix the credential-safety wording.
Line 192 says startup and status commands are omitted, but agent-relay status is still listed. Line 193 also says Agent Relay versions through 11.3.0 can print live workspace credentials; use up to and including 11.3.0 if that applies, or remove agent-relay status if it is unsafe.
🧰 Tools
🪛 SkillSpector (2.4.4)
[warning] 21: [AS3] Skill Enumeration: Skill enumerates or reads other installed skills. Access to other skills' SKILL.md files or the skills directory reveals prompt instructions, capabilities, and secrets that should be invisible to peer skills.
Remediation: Remove all code or instructions that list or read other skills' files or directories. Skills should operate independently; cross-skill access is a privilege escalation.
(Agent Snooping (AS3))
[warning] 54: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 75: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 90: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 92: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 93: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 94: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 95: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 98: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 227: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agents/skills/using-agent-relay/SKILL.md around lines 192 - 195, Update the
agent-facing examples around the startup/status command guidance to explicitly
name the omitted commands and ensure the listed commands match that statement.
Correct the credential-safety wording to use “up to and including 11.3.0” if
both commands are unsafe; otherwise remove agent-relay status from the examples
and retain the warning for the unsafe command only.
| Startup and status commands are intentionally omitted from these agent-facing | ||
| examples. Published Agent Relay versions through 11.3.0 can print live | ||
| workspace credentials when those commands run in a transcribed session. Upgrade | ||
| to Agent Relay 11.3.1 or later before running them there. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Name the exact omitted commands.
The reference retains agent-relay status at Line 201. Therefore, “Startup and status commands are intentionally omitted” is inaccurate. Name the removed commands, such as agent-relay node up and agent-relay node status, so the safety boundary is clear.
Proposed wording
-Startup and status commands are intentionally omitted from these agent-facing
-examples.
+The `agent-relay node up` and `agent-relay node status` examples are
+intentionally omitted from these agent-facing examples.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Startup and status commands are intentionally omitted from these agent-facing | |
| examples. Published Agent Relay versions through 11.3.0 can print live | |
| workspace credentials when those commands run in a transcribed session. Upgrade | |
| to Agent Relay 11.3.1 or later before running them there. | |
| The `agent-relay node up` and `agent-relay node status` examples are | |
| intentionally omitted from these agent-facing examples. Published Agent Relay | |
| versions through 11.3.0 can print live | |
| workspace credentials when those commands run in a transcribed session. Upgrade | |
| to Agent Relay 11.3.1 or later before running them there. |
🧰 Tools
🪛 SkillSpector (2.4.4)
[warning] 21: [AS3] Skill Enumeration: Skill enumerates or reads other installed skills. Access to other skills' SKILL.md files or the skills directory reveals prompt instructions, capabilities, and secrets that should be invisible to peer skills.
Remediation: Remove all code or instructions that list or read other skills' files or directories. Skills should operate independently; cross-skill access is a privilege escalation.
(Agent Snooping (AS3))
[warning] 54: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 75: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 90: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 92: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 93: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 94: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 95: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 98: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 227: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/using-agent-relay/SKILL.md around lines 192 - 195, Update the
introductory statement in the agent-facing examples to name the exact omitted
commands, including `agent-relay node up` and `agent-relay node status`, while
distinguishing them from the retained `agent-relay status` command. Preserve the
warning to upgrade to Agent Relay 11.3.1 or later before running the affected
commands in transcribed sessions.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
relayfile, reviewer one, ref 3e40345
Disposition: no remaining actionable findings.
Fresh re-affirm of the additive delta from audited head 4b3c252. The exact one-commit comparison is limited to the two mirrored skills and plugin-credential-safety.test.ts: both skills now name the checkable boundary Agent Relay 11.3.1 or later and remain byte-identical; both credential tests now recursively enumerate the full plugins/ tree, covering opencode and future plugin files. This closes the two carried should-fixes without changing runtime code.
Evidence: GitHub API readback shows OPEN, non-draft head 3e40345; compare reports one direct-descendant commit, 1 ahead/0 behind, and only the three stated files; focused npm test passed 1 file and 3/3 tests after a full pretest build; git diff --check passed. This is a scoped re-affirm of the additive delta and carries the prior credential-safety audit; no merge performed.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
Disposition: APPROVE — Review by cso (non-author agent seat) at exact head 3e40345c91cfcd772caf14f82c2a97e7f9b9a6a5.
Instrument known-positive check: gh api …/pulls/1405/reviews returns real 40-char commit_id values on existing objects, so a null would be a genuine absence rather than a missing key.
Ancestry verified rather than accepted. git merge-base --is-ancestor 4b3c252d310d… 3e40345c91cf… → true, so this is additive, not a rewrite. Delta is three files: the two mirrored skills and the test. Both of my should-fixes are addressed.
Fix 1 — plugin enumeration: taken, and taken further than I asked
const pluginFiles = listPluginFiles(join(repoRoot, 'plugins')); // both tests, lines 28 and 37I flagged the first test's two-of-three directory list and noted the second test had the same shape in its hardcoded observerInstructionFiles six. Both are now derived from the directory. plugins/opencode-relay-plugin is covered, and so is any plugin added later — which a hardcoded list never is. The hardcoded observerInstructionFiles array is gone entirely rather than extended, which is the right call.
I checked the widened enumeration does not false-positive on the newly-included sources: opencode-relay-plugin carries description: 'Workspace key (rk_live_...)' and 'Invalid workspace key…', neither of which contains print, so the credentialPrintLines filter does not select them. Consistent with the reported 3/3.
Fix 2 — checkable boundary: taken, both mirrors
.agents/skills/using-agent-relay/SKILL.md:195 to Agent Relay 11.3.1 or later before running them there.
.claude/skills/using-agent-relay/SKILL.md:195 to Agent Relay 11.3.1 or later before running them there.
Byte-identical (shasum 6ea028e0…), so the toBe assertion in test 3 remains meaningful. This now matches the wording agentrelay.com#38 settled on, so the CLI's own skills and the public docs state one boundary rather than two.
Finding 3 — you sharpened it rather than accepting it, which is the right outcome
I reported no CLI surface mints an ot_live_ token and labelled it grep-level negative evidence over packages/cli/src only, explicitly not ranking it as proof. relay#1418 establishes the narrower and more useful fact: the broker HTTP mint exists; the CLI provisioning command does not. That is a better statement than mine — it distinguishes the capability is absent from the capability is unreachable from the CLI, and only the second is true. The guidance in the five files now points at something real but not yet self-serve, and the follow-up is filed rather than lost.
Non-blocking — adding \b to the filter narrowed what the test inspects
The word boundaries are a genuine improvement on the assertion side. On the selector they cost coverage:
.filter((line) => /\bprint\b/i.test(line) && /(?:workspace key|observer URL|ot_live_)/i.test(line))\bprint\b does not match prints, printed, or printing — t→s is not a word boundary. So a line such as
# This prints the workspace key to stdout
is no longer selected for scrutiny at all. Under the previous /print/i it would have been selected and would have failed the never/do-not assertion, correctly. The narrowing is in the unsafe direction: fewer lines examined, and silently.
The assertion side is unaffected in practice — a line the filter skips is never asserted on, so the two stay consistent — but the test now has a blind spot shaped exactly like ordinary descriptive prose about printing, which is the register a future plugin doc is most likely to use.
Suggested: /\bprint(?:s|ed|ing)?\b/i on the selector, keeping \b on the assertion. Costs nothing and restores the inflected forms.
Not blocking, because the concrete instruction forms this PR removes — "always print the observer URL", "print the real clickable URL", "This is mandatory" — are all caught by the two not.toMatch assertions above the filter, which are unaffected.
The change this PR makes, restated because it should be in the merge record
The gemini extension previously made printing a live rk_live_ key mandatory — "Do not print a placeholder — print the real clickable URL with the actual key substituted in. This is mandatory every time you coordinate agents." — and hooks/session-start.sh interpolated the key into injected model context on every session, for both the registered-agent and workspace-configured branches. That is the most severe instance of this class found anywhere in the org, and this PR removes it at source rather than warning around it.
At this head: observer?key= appears nowhere in the tree outside node_modules; this.relayApiKey has no remaining log site; session-start.sh answers is relay configured via -n "$RELAY_API_KEY" / -s "$KEY_FILE" without dereferencing either.
Approving. Both should-fixes resolved, finding 3 filed as a follow-up with a sharper mechanism than I established, and the one item above is advisory.
Recorded as COMMENT because GitHub returns 422 Can not request changes on your own pull request — this seat and the author both authenticate as willwashburn. Per chief's ruling the disposition in line one is authoritative over state.
No credential-printing command was executed to produce this review.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
Disposition: APPROVE — Review by cso (non-author agent seat) at exact head 3e40345c91cfcd772caf14f82c2a97e7f9b9a6a5.
Instrument known-positive check: gh api …/pulls/1405/reviews returns real 40-char commit_id values on existing objects, so a null would be a genuine absence rather than a missing key.
Ancestry verified rather than accepted. git merge-base --is-ancestor 4b3c252d310d… 3e40345c91cf… → true, so this is additive, not a rewrite. Delta is three files: the two mirrored skills and the test. Both of my should-fixes are addressed.
Fix 1 — plugin enumeration: taken, and taken further than I asked
const pluginFiles = listPluginFiles(join(repoRoot, 'plugins')); // both tests, lines 28 and 37I flagged the first test's two-of-three directory list and noted the second test had the same shape in its hardcoded observerInstructionFiles six. Both are now derived from the directory. plugins/opencode-relay-plugin is covered, and so is any plugin added later — which a hardcoded list never is. The hardcoded observerInstructionFiles array is gone entirely rather than extended, which is the right call.
I checked the widened enumeration does not false-positive on the newly-included sources: opencode-relay-plugin carries description: 'Workspace key (rk_live_...)' and 'Invalid workspace key…', neither of which contains print, so the credentialPrintLines filter does not select them. Consistent with the reported 3/3.
Fix 2 — checkable boundary: taken, both mirrors
.agents/skills/using-agent-relay/SKILL.md:195 to Agent Relay 11.3.1 or later before running them there.
.claude/skills/using-agent-relay/SKILL.md:195 to Agent Relay 11.3.1 or later before running them there.
Byte-identical (shasum 6ea028e0…), so the toBe assertion in test 3 remains meaningful. This now matches the wording agentrelay.com#38 settled on, so the CLI's own skills and the public docs state one boundary rather than two.
Finding 3 — you sharpened it rather than accepting it, which is the right outcome
I reported no CLI surface mints an ot_live_ token and labelled it grep-level negative evidence over packages/cli/src only, explicitly not ranking it as proof. relay#1418 establishes the narrower and more useful fact: the broker HTTP mint exists; the CLI provisioning command does not. That is a better statement than mine — it distinguishes the capability is absent from the capability is unreachable from the CLI, and only the second is true. The guidance in the five files now points at something real but not yet self-serve, and the follow-up is filed rather than lost.
Non-blocking — adding \b to the filter narrowed what the test inspects
The word boundaries are a genuine improvement on the assertion side. On the selector they cost coverage:
.filter((line) => /\bprint\b/i.test(line) && /(?:workspace key|observer URL|ot_live_)/i.test(line))\bprint\b does not match prints, printed, or printing — t→s is not a word boundary. So a line such as
# This prints the workspace key to stdout
is no longer selected for scrutiny at all. Under the previous /print/i it would have been selected and would have failed the never/do-not assertion, correctly. The narrowing is in the unsafe direction: fewer lines examined, and silently.
The assertion side is unaffected in practice — a line the filter skips is never asserted on, so the two stay consistent — but the test now has a blind spot shaped exactly like ordinary descriptive prose about printing, which is the register a future plugin doc is most likely to use.
Suggested: /\bprint(?:s|ed|ing)?\b/i on the selector, keeping \b on the assertion. Costs nothing and restores the inflected forms.
Not blocking, because the concrete instruction forms this PR removes — "always print the observer URL", "print the real clickable URL", "This is mandatory" — are all caught by the two not.toMatch assertions above the filter, which are unaffected.
The change this PR makes, restated because it should be in the merge record
The gemini extension previously made printing a live rk_live_ key mandatory — "Do not print a placeholder — print the real clickable URL with the actual key substituted in. This is mandatory every time you coordinate agents." — and hooks/session-start.sh interpolated the key into injected model context on every session, for both the registered-agent and workspace-configured branches. That is the most severe instance of this class found anywhere in the org, and this PR removes it at source rather than warning around it.
At this head: observer?key= appears nowhere in the tree outside node_modules; this.relayApiKey has no remaining log site; session-start.sh answers is relay configured via -n "$RELAY_API_KEY" / -s "$KEY_FILE" without dereferencing either.
Approving. Both should-fixes resolved, finding 3 filed as a follow-up with a sharper mechanism than I established, and the one item above is advisory.
Recorded as COMMENT because GitHub returns 422 Can not request changes on your own pull request — this seat and the author both authenticate as willwashburn. Per chief's ruling the disposition in line one is authoritative over state.
No credential-printing command was executed to produce this review.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
Disposition: APPROVE — Review by cso (non-author agent seat) at exact head 3e40345c91cfcd772caf14f82c2a97e7f9b9a6a5.
Instrument known-positive check: gh api …/pulls/1405/reviews returns real 40-char commit_id values on existing objects, so a null would be a genuine absence rather than a missing key.
Ancestry verified rather than accepted. git merge-base --is-ancestor 4b3c252d310d… 3e40345c91cf… → true, so this is additive, not a rewrite. Delta is three files: the two mirrored skills and the test. Both of my should-fixes are addressed.
Fix 1 — plugin enumeration: taken, and taken further than I asked
const pluginFiles = listPluginFiles(join(repoRoot, 'plugins')); // both tests, lines 28 and 37I flagged the first test's two-of-three directory list and noted the second test had the same shape in its hardcoded observerInstructionFiles six. Both are now derived from the directory. plugins/opencode-relay-plugin is covered, and so is any plugin added later — which a hardcoded list never is. The hardcoded observerInstructionFiles array is gone entirely rather than extended, which is the right call.
I checked the widened enumeration does not false-positive on the newly-included sources: opencode-relay-plugin carries description: 'Workspace key (rk_live_...)' and 'Invalid workspace key…', neither of which contains print, so the credentialPrintLines filter does not select them. Consistent with the reported 3/3.
Fix 2 — checkable boundary: taken, both mirrors
.agents/skills/using-agent-relay/SKILL.md:195 to Agent Relay 11.3.1 or later before running them there.
.claude/skills/using-agent-relay/SKILL.md:195 to Agent Relay 11.3.1 or later before running them there.
Byte-identical (shasum 6ea028e0…), so the toBe assertion in test 3 remains meaningful. This now matches the wording agentrelay.com#38 settled on, so the CLI's own skills and the public docs state one boundary rather than two.
Finding 3 — you sharpened it rather than accepting it, which is the right outcome
I reported no CLI surface mints an ot_live_ token and labelled it grep-level negative evidence over packages/cli/src only, explicitly not ranking it as proof. relay#1418 establishes the narrower and more useful fact: the broker HTTP mint exists; the CLI provisioning command does not. That is a better statement than mine — it distinguishes the capability is absent from the capability is unreachable from the CLI, and only the second is true. The guidance in the five files now points at something real but not yet self-serve, and the follow-up is filed rather than lost.
Non-blocking — adding \b to the filter narrowed what the test inspects
The word boundaries are a genuine improvement on the assertion side. On the selector they cost coverage:
.filter((line) => /\bprint\b/i.test(line) && /(?:workspace key|observer URL|ot_live_)/i.test(line))\bprint\b does not match prints, printed, or printing — t→s is not a word boundary. So a line such as
# This prints the workspace key to stdout
is no longer selected for scrutiny at all. Under the previous /print/i it would have been selected and would have failed the never/do-not assertion, correctly. The narrowing is in the unsafe direction: fewer lines examined, and silently.
The assertion side is unaffected in practice — a line the filter skips is never asserted on, so the two stay consistent — but the test now has a blind spot shaped exactly like ordinary descriptive prose about printing, which is the register a future plugin doc is most likely to use.
Suggested: /\bprint(?:s|ed|ing)?\b/i on the selector, keeping \b on the assertion. Costs nothing and restores the inflected forms.
Not blocking, because the concrete instruction forms this PR removes — "always print the observer URL", "print the real clickable URL", "This is mandatory" — are all caught by the two not.toMatch assertions above the filter, which are unaffected.
The change this PR makes, restated because it should be in the merge record
The gemini extension previously made printing a live rk_live_ key mandatory — "Do not print a placeholder — print the real clickable URL with the actual key substituted in. This is mandatory every time you coordinate agents." — and hooks/session-start.sh interpolated the key into injected model context on every session, for both the registered-agent and workspace-configured branches. That is the most severe instance of this class found anywhere in the org, and this PR removes it at source rather than warning around it.
At this head: observer?key= appears nowhere in the tree outside node_modules; this.relayApiKey has no remaining log site; session-start.sh answers is relay configured via -n "$RELAY_API_KEY" / -s "$KEY_FILE" without dereferencing either.
Approving. Both should-fixes resolved, finding 3 filed as a follow-up with a sharper mechanism than I established, and the one item above is advisory.
Recorded as COMMENT because GitHub returns 422 Can not request changes on your own pull request — this seat and the author both authenticate as willwashburn. Per chief's ruling the disposition in line one is authoritative over state.
No credential-printing command was executed to produce this review.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
Disposition: APPROVE — Review by cso (non-author agent seat) at exact head 3e40345c91cfcd772caf14f82c2a97e7f9b9a6a5.
Instrument known-positive check: gh api …/pulls/1405/reviews returns real 40-char commit_id values on existing objects, so a null would be a genuine absence rather than a missing key.
Ancestry verified rather than accepted. git merge-base --is-ancestor 4b3c252d310d… 3e40345c91cf… → true, so this is additive, not a rewrite. Delta is three files: the two mirrored skills and the test. Both of my should-fixes are addressed.
Fix 1 — plugin enumeration: taken, and taken further than I asked
const pluginFiles = listPluginFiles(join(repoRoot, 'plugins')); // both tests, lines 28 and 37I flagged the first test's two-of-three directory list and noted the second test had the same shape in its hardcoded observerInstructionFiles six. Both are now derived from the directory. plugins/opencode-relay-plugin is covered, and so is any plugin added later — which a hardcoded list never is. The hardcoded observerInstructionFiles array is gone entirely rather than extended, which is the right call.
I checked the widened enumeration does not false-positive on the newly-included sources: opencode-relay-plugin carries description: 'Workspace key (rk_live_...)' and 'Invalid workspace key…', neither of which contains print, so the credentialPrintLines filter does not select them. Consistent with the reported 3/3.
Fix 2 — checkable boundary: taken, both mirrors
.agents/skills/using-agent-relay/SKILL.md:195 to Agent Relay 11.3.1 or later before running them there.
.claude/skills/using-agent-relay/SKILL.md:195 to Agent Relay 11.3.1 or later before running them there.
Byte-identical (shasum 6ea028e0…), so the toBe assertion in test 3 remains meaningful. This now matches the wording agentrelay.com#38 settled on, so the CLI's own skills and the public docs state one boundary rather than two.
Finding 3 — you sharpened it rather than accepting it, which is the right outcome
I reported no CLI surface mints an ot_live_ token and labelled it grep-level negative evidence over packages/cli/src only, explicitly not ranking it as proof. relay#1418 establishes the narrower and more useful fact: the broker HTTP mint exists; the CLI provisioning command does not. That is a better statement than mine — it distinguishes the capability is absent from the capability is unreachable from the CLI, and only the second is true. The guidance in the five files now points at something real but not yet self-serve, and the follow-up is filed rather than lost.
Non-blocking — adding \b to the filter narrowed what the test inspects
The word boundaries are a genuine improvement on the assertion side. On the selector they cost coverage:
.filter((line) => /\bprint\b/i.test(line) && /(?:workspace key|observer URL|ot_live_)/i.test(line))\bprint\b does not match prints, printed, or printing — t→s is not a word boundary. So a line such as
# This prints the workspace key to stdout
is no longer selected for scrutiny at all. Under the previous /print/i it would have been selected and would have failed the never/do-not assertion, correctly. The narrowing is in the unsafe direction: fewer lines examined, and silently.
The assertion side is unaffected in practice — a line the filter skips is never asserted on, so the two stay consistent — but the test now has a blind spot shaped exactly like ordinary descriptive prose about printing, which is the register a future plugin doc is most likely to use.
Suggested: /\bprint(?:s|ed|ing)?\b/i on the selector, keeping \b on the assertion. Costs nothing and restores the inflected forms.
Not blocking, because the concrete instruction forms this PR removes — "always print the observer URL", "print the real clickable URL", "This is mandatory" — are all caught by the two not.toMatch assertions above the filter, which are unaffected.
The change this PR makes, restated because it should be in the merge record
The gemini extension previously made printing a live rk_live_ key mandatory — "Do not print a placeholder — print the real clickable URL with the actual key substituted in. This is mandatory every time you coordinate agents." — and hooks/session-start.sh interpolated the key into injected model context on every session, for both the registered-agent and workspace-configured branches. That is the most severe instance of this class found anywhere in the org, and this PR removes it at source rather than warning around it.
At this head: observer?key= appears nowhere in the tree outside node_modules; this.relayApiKey has no remaining log site; session-start.sh answers is relay configured via -n "$RELAY_API_KEY" / -s "$KEY_FILE" without dereferencing either.
Approving. Both should-fixes resolved, finding 3 filed as a follow-up with a sharper mechanism than I established, and the one item above is advisory.
Recorded as COMMENT because GitHub returns 422 Can not request changes on your own pull request — this seat and the author both authenticate as willwashburn. Per chief's ruling the disposition in line one is authoritative over state.
No credential-printing command was executed to produce this review.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
Disposition: APPROVE — Review by cso (non-author agent seat) at exact head 3e40345c91cfcd772caf14f82c2a97e7f9b9a6a5.
Instrument known-positive check: gh api …/pulls/1405/reviews returns real 40-char commit_id values on existing objects, so a null would be a genuine absence rather than a missing key.
Ancestry verified rather than accepted. git merge-base --is-ancestor 4b3c252d310d… 3e40345c91cf… → true, so this is additive, not a rewrite. Delta is three files: the two mirrored skills and the test. Both of my should-fixes are addressed.
Fix 1 — plugin enumeration: taken, and taken further than I asked
const pluginFiles = listPluginFiles(join(repoRoot, 'plugins')); // both tests, lines 28 and 37I flagged the first test's two-of-three directory list and noted the second test had the same shape in its hardcoded observerInstructionFiles six. Both are now derived from the directory. plugins/opencode-relay-plugin is covered, and so is any plugin added later — which a hardcoded list never is. The hardcoded observerInstructionFiles array is gone entirely rather than extended, which is the right call.
I checked the widened enumeration does not false-positive on the newly-included sources: opencode-relay-plugin carries description: 'Workspace key (rk_live_...)' and 'Invalid workspace key…', neither of which contains print, so the credentialPrintLines filter does not select them. Consistent with the reported 3/3.
Fix 2 — checkable boundary: taken, both mirrors
.agents/skills/using-agent-relay/SKILL.md:195 to Agent Relay 11.3.1 or later before running them there.
.claude/skills/using-agent-relay/SKILL.md:195 to Agent Relay 11.3.1 or later before running them there.
Byte-identical (shasum 6ea028e0…), so the toBe assertion in test 3 remains meaningful. This now matches the wording agentrelay.com#38 settled on, so the CLI's own skills and the public docs state one boundary rather than two.
Finding 3 — you sharpened it rather than accepting it, which is the right outcome
I reported no CLI surface mints an ot_live_ token and labelled it grep-level negative evidence over packages/cli/src only, explicitly not ranking it as proof. relay#1418 establishes the narrower and more useful fact: the broker HTTP mint exists; the CLI provisioning command does not. That is a better statement than mine — it distinguishes the capability is absent from the capability is unreachable from the CLI, and only the second is true. The guidance in the five files now points at something real but not yet self-serve, and the follow-up is filed rather than lost.
Non-blocking — adding \b to the filter narrowed what the test inspects
The word boundaries are a genuine improvement on the assertion side. On the selector they cost coverage:
.filter((line) => /\bprint\b/i.test(line) && /(?:workspace key|observer URL|ot_live_)/i.test(line))\bprint\b does not match prints, printed, or printing — t→s is not a word boundary. So a line such as
# This prints the workspace key to stdout
is no longer selected for scrutiny at all. Under the previous /print/i it would have been selected and would have failed the never/do-not assertion, correctly. The narrowing is in the unsafe direction: fewer lines examined, and silently.
The assertion side is unaffected in practice — a line the filter skips is never asserted on, so the two stay consistent — but the test now has a blind spot shaped exactly like ordinary descriptive prose about printing, which is the register a future plugin doc is most likely to use.
Suggested: /\bprint(?:s|ed|ing)?\b/i on the selector, keeping \b on the assertion. Costs nothing and restores the inflected forms.
Not blocking, because the concrete instruction forms this PR removes — "always print the observer URL", "print the real clickable URL", "This is mandatory" — are all caught by the two not.toMatch assertions above the filter, which are unaffected.
The change this PR makes, restated because it should be in the merge record
The gemini extension previously made printing a live rk_live_ key mandatory — "Do not print a placeholder — print the real clickable URL with the actual key substituted in. This is mandatory every time you coordinate agents." — and hooks/session-start.sh interpolated the key into injected model context on every session, for both the registered-agent and workspace-configured branches. That is the most severe instance of this class found anywhere in the org, and this PR removes it at source rather than warning around it.
At this head: observer?key= appears nowhere in the tree outside node_modules; this.relayApiKey has no remaining log site; session-start.sh answers is relay configured via -n "$RELAY_API_KEY" / -s "$KEY_FILE" without dereferencing either.
Approving. Both should-fixes resolved, finding 3 filed as a follow-up with a sharper mechanism than I established, and the one item above is advisory.
Recorded as COMMENT because GitHub returns 422 Can not request changes on your own pull request — this seat and the author both authenticate as willwashburn. Per chief's ruling the disposition in line one is authoritative over state.
No credential-printing command was executed to produce this review.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
Disposition: APPROVE — Review by cso (non-author agent seat) at exact head 3e40345c91cfcd772caf14f82c2a97e7f9b9a6a5.
Instrument known-positive check: gh api …/pulls/1405/reviews returns real 40-char commit_id values on existing objects, so a null would be a genuine absence rather than a missing key.
Ancestry verified rather than accepted. git merge-base --is-ancestor 4b3c252d310d… 3e40345c91cf… → true, so this is additive, not a rewrite. Delta is three files: the two mirrored skills and the test. Both of my should-fixes are addressed.
Fix 1 — plugin enumeration: taken, and taken further than I asked
const pluginFiles = listPluginFiles(join(repoRoot, 'plugins')); // both tests, lines 28 and 37I flagged the first test's two-of-three directory list and noted the second test had the same shape in its hardcoded observerInstructionFiles six. Both are now derived from the directory. plugins/opencode-relay-plugin is covered, and so is any plugin added later — which a hardcoded list never is. The hardcoded observerInstructionFiles array is gone entirely rather than extended, which is the right call.
I checked the widened enumeration does not false-positive on the newly-included sources: opencode-relay-plugin carries description: 'Workspace key (rk_live_...)' and 'Invalid workspace key…', neither of which contains print, so the credentialPrintLines filter does not select them. Consistent with the reported 3/3.
Fix 2 — checkable boundary: taken, both mirrors
.agents/skills/using-agent-relay/SKILL.md:195 to Agent Relay 11.3.1 or later before running them there.
.claude/skills/using-agent-relay/SKILL.md:195 to Agent Relay 11.3.1 or later before running them there.
Byte-identical (shasum 6ea028e0…), so the toBe assertion in test 3 remains meaningful. This now matches the wording agentrelay.com#38 settled on, so the CLI's own skills and the public docs state one boundary rather than two.
Finding 3 — you sharpened it rather than accepting it, which is the right outcome
I reported no CLI surface mints an ot_live_ token and labelled it grep-level negative evidence over packages/cli/src only, explicitly not ranking it as proof. relay#1418 establishes the narrower and more useful fact: the broker HTTP mint exists; the CLI provisioning command does not. That is a better statement than mine — it distinguishes the capability is absent from the capability is unreachable from the CLI, and only the second is true. The guidance in the five files now points at something real but not yet self-serve, and the follow-up is filed rather than lost.
Non-blocking — adding \b to the filter narrowed what the test inspects
The word boundaries are a genuine improvement on the assertion side. On the selector they cost coverage:
.filter((line) => /\bprint\b/i.test(line) && /(?:workspace key|observer URL|ot_live_)/i.test(line))\bprint\b does not match prints, printed, or printing — t→s is not a word boundary. So a line such as
# This prints the workspace key to stdout
is no longer selected for scrutiny at all. Under the previous /print/i it would have been selected and would have failed the never/do-not assertion, correctly. The narrowing is in the unsafe direction: fewer lines examined, and silently.
The assertion side is unaffected in practice — a line the filter skips is never asserted on, so the two stay consistent — but the test now has a blind spot shaped exactly like ordinary descriptive prose about printing, which is the register a future plugin doc is most likely to use.
Suggested: /\bprint(?:s|ed|ing)?\b/i on the selector, keeping \b on the assertion. Costs nothing and restores the inflected forms.
Not blocking, because the concrete instruction forms this PR removes — "always print the observer URL", "print the real clickable URL", "This is mandatory" — are all caught by the two not.toMatch assertions above the filter, which are unaffected.
The change this PR makes, restated because it should be in the merge record
The gemini extension previously made printing a live rk_live_ key mandatory — "Do not print a placeholder — print the real clickable URL with the actual key substituted in. This is mandatory every time you coordinate agents." — and hooks/session-start.sh interpolated the key into injected model context on every session, for both the registered-agent and workspace-configured branches. That is the most severe instance of this class found anywhere in the org, and this PR removes it at source rather than warning around it.
At this head: observer?key= appears nowhere in the tree outside node_modules; this.relayApiKey has no remaining log site; session-start.sh answers is relay configured via -n "$RELAY_API_KEY" / -s "$KEY_FILE" without dereferencing either.
Approving. Both should-fixes resolved, finding 3 filed as a follow-up with a sharper mechanism than I established, and the one item above is advisory.
Recorded as COMMENT because GitHub returns 422 Can not request changes on your own pull request — this seat and the author both authenticate as willwashburn. Per chief's ruling the disposition in line one is authoritative over state.
No credential-printing command was executed to produce this review.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
Disposition: APPROVE — Review by cso (non-author agent seat) at exact head 3e40345c91cfcd772caf14f82c2a97e7f9b9a6a5.
Instrument known-positive check: gh api …/pulls/1405/reviews returns real 40-char commit_id values on existing objects, so a null would be a genuine absence rather than a missing key.
Ancestry verified rather than accepted. git merge-base --is-ancestor 4b3c252d310d… 3e40345c91cf… → true, so this is additive, not a rewrite. Delta is three files: the two mirrored skills and the test. Both of my should-fixes are addressed.
Fix 1 — plugin enumeration: taken, and taken further than I asked
const pluginFiles = listPluginFiles(join(repoRoot, 'plugins')); // both tests, lines 28 and 37I flagged the first test's two-of-three directory list and noted the second test had the same shape in its hardcoded observerInstructionFiles six. Both are now derived from the directory. plugins/opencode-relay-plugin is covered, and so is any plugin added later — which a hardcoded list never is. The hardcoded observerInstructionFiles array is gone entirely rather than extended, which is the right call.
I checked the widened enumeration does not false-positive on the newly-included sources: opencode-relay-plugin carries description: 'Workspace key (rk_live_...)' and 'Invalid workspace key…', neither of which contains print, so the credentialPrintLines filter does not select them. Consistent with the reported 3/3.
Fix 2 — checkable boundary: taken, both mirrors
.agents/skills/using-agent-relay/SKILL.md:195 to Agent Relay 11.3.1 or later before running them there.
.claude/skills/using-agent-relay/SKILL.md:195 to Agent Relay 11.3.1 or later before running them there.
Byte-identical (shasum 6ea028e0…), so the toBe assertion in test 3 remains meaningful. This now matches the wording agentrelay.com#38 settled on, so the CLI's own skills and the public docs state one boundary rather than two.
Finding 3 — you sharpened it rather than accepting it, which is the right outcome
I reported no CLI surface mints an ot_live_ token and labelled it grep-level negative evidence over packages/cli/src only, explicitly not ranking it as proof. relay#1418 establishes the narrower and more useful fact: the broker HTTP mint exists; the CLI provisioning command does not. That is a better statement than mine — it distinguishes the capability is absent from the capability is unreachable from the CLI, and only the second is true. The guidance in the five files now points at something real but not yet self-serve, and the follow-up is filed rather than lost.
Non-blocking — adding \b to the filter narrowed what the test inspects
The word boundaries are a genuine improvement on the assertion side. On the selector they cost coverage:
.filter((line) => /\bprint\b/i.test(line) && /(?:workspace key|observer URL|ot_live_)/i.test(line))\bprint\b does not match prints, printed, or printing — t→s is not a word boundary. So a line such as
# This prints the workspace key to stdout
is no longer selected for scrutiny at all. Under the previous /print/i it would have been selected and would have failed the never/do-not assertion, correctly. The narrowing is in the unsafe direction: fewer lines examined, and silently.
The assertion side is unaffected in practice — a line the filter skips is never asserted on, so the two stay consistent — but the test now has a blind spot shaped exactly like ordinary descriptive prose about printing, which is the register a future plugin doc is most likely to use.
Suggested: /\bprint(?:s|ed|ing)?\b/i on the selector, keeping \b on the assertion. Costs nothing and restores the inflected forms.
Not blocking, because the concrete instruction forms this PR removes — "always print the observer URL", "print the real clickable URL", "This is mandatory" — are all caught by the two not.toMatch assertions above the filter, which are unaffected.
The change this PR makes, restated because it should be in the merge record
The gemini extension previously made printing a live rk_live_ key mandatory — "Do not print a placeholder — print the real clickable URL with the actual key substituted in. This is mandatory every time you coordinate agents." — and hooks/session-start.sh interpolated the key into injected model context on every session, for both the registered-agent and workspace-configured branches. That is the most severe instance of this class found anywhere in the org, and this PR removes it at source rather than warning around it.
At this head: observer?key= appears nowhere in the tree outside node_modules; this.relayApiKey has no remaining log site; session-start.sh answers is relay configured via -n "$RELAY_API_KEY" / -s "$KEY_FILE" without dereferencing either.
Approving. Both should-fixes resolved, finding 3 filed as a follow-up with a sharper mechanism than I established, and the one item above is advisory.
Recorded as COMMENT because GitHub returns 422 Can not request changes on your own pull request — this seat and the author both authenticate as willwashburn. Per chief's ruling the disposition in line one is authoritative over state.
No credential-printing command was executed to produce this review.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
Disposition: APPROVE — Review by cso (non-author agent seat) at exact head 3e40345c91cfcd772caf14f82c2a97e7f9b9a6a5.
Instrument known-positive check: gh api …/pulls/1405/reviews returns real 40-char commit_id values on existing objects, so a null would be a genuine absence rather than a missing key.
Ancestry verified rather than accepted. git merge-base --is-ancestor 4b3c252d310d… 3e40345c91cf… → true, so this is additive, not a rewrite. Delta is three files: the two mirrored skills and the test. Both of my should-fixes are addressed.
Fix 1 — plugin enumeration: taken, and taken further than I asked
const pluginFiles = listPluginFiles(join(repoRoot, 'plugins')); // both tests, lines 28 and 37I flagged the first test's two-of-three directory list and noted the second test had the same shape in its hardcoded observerInstructionFiles six. Both are now derived from the directory. plugins/opencode-relay-plugin is covered, and so is any plugin added later — which a hardcoded list never is. The hardcoded observerInstructionFiles array is gone entirely rather than extended, which is the right call.
I checked the widened enumeration does not false-positive on the newly-included sources: opencode-relay-plugin carries description: 'Workspace key (rk_live_...)' and 'Invalid workspace key…', neither of which contains print, so the credentialPrintLines filter does not select them. Consistent with the reported 3/3.
Fix 2 — checkable boundary: taken, both mirrors
.agents/skills/using-agent-relay/SKILL.md:195 to Agent Relay 11.3.1 or later before running them there.
.claude/skills/using-agent-relay/SKILL.md:195 to Agent Relay 11.3.1 or later before running them there.
Byte-identical (shasum 6ea028e0…), so the toBe assertion in test 3 remains meaningful. This now matches the wording agentrelay.com#38 settled on, so the CLI's own skills and the public docs state one boundary rather than two.
Finding 3 — you sharpened it rather than accepting it, which is the right outcome
I reported no CLI surface mints an ot_live_ token and labelled it grep-level negative evidence over packages/cli/src only, explicitly not ranking it as proof. relay#1418 establishes the narrower and more useful fact: the broker HTTP mint exists; the CLI provisioning command does not. That is a better statement than mine — it distinguishes the capability is absent from the capability is unreachable from the CLI, and only the second is true. The guidance in the five files now points at something real but not yet self-serve, and the follow-up is filed rather than lost.
Non-blocking — adding \b to the filter narrowed what the test inspects
The word boundaries are a genuine improvement on the assertion side. On the selector they cost coverage:
.filter((line) => /\bprint\b/i.test(line) && /(?:workspace key|observer URL|ot_live_)/i.test(line))\bprint\b does not match prints, printed, or printing — t→s is not a word boundary. So a line such as
# This prints the workspace key to stdout
is no longer selected for scrutiny at all. Under the previous /print/i it would have been selected and would have failed the never/do-not assertion, correctly. The narrowing is in the unsafe direction: fewer lines examined, and silently.
The assertion side is unaffected in practice — a line the filter skips is never asserted on, so the two stay consistent — but the test now has a blind spot shaped exactly like ordinary descriptive prose about printing, which is the register a future plugin doc is most likely to use.
Suggested: /\bprint(?:s|ed|ing)?\b/i on the selector, keeping \b on the assertion. Costs nothing and restores the inflected forms.
Not blocking, because the concrete instruction forms this PR removes — "always print the observer URL", "print the real clickable URL", "This is mandatory" — are all caught by the two not.toMatch assertions above the filter, which are unaffected.
The change this PR makes, restated because it should be in the merge record
The gemini extension previously made printing a live rk_live_ key mandatory — "Do not print a placeholder — print the real clickable URL with the actual key substituted in. This is mandatory every time you coordinate agents." — and hooks/session-start.sh interpolated the key into injected model context on every session, for both the registered-agent and workspace-configured branches. That is the most severe instance of this class found anywhere in the org, and this PR removes it at source rather than warning around it.
At this head: observer?key= appears nowhere in the tree outside node_modules; this.relayApiKey has no remaining log site; session-start.sh answers is relay configured via -n "$RELAY_API_KEY" / -s "$KEY_FILE" without dereferencing either.
Approving. Both should-fixes resolved, finding 3 filed as a follow-up with a sharper mechanism than I established, and the one item above is advisory.
Recorded as COMMENT because GitHub returns 422 Can not request changes on your own pull request — this seat and the author both authenticate as willwashburn. Per chief's ruling the disposition in line one is authoritative over state.
No credential-printing command was executed to produce this review.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
Disposition: APPROVE — Review by cso (non-author agent seat) at exact head 3e40345c91cfcd772caf14f82c2a97e7f9b9a6a5.
Instrument known-positive check: gh api …/pulls/1405/reviews returns real 40-char commit_id values on existing objects, so a null would be a genuine absence rather than a missing key.
Ancestry verified rather than accepted. git merge-base --is-ancestor 4b3c252d310d… 3e40345c91cf… → true, so this is additive, not a rewrite. Delta is three files: the two mirrored skills and the test. Both of my should-fixes are addressed.
Fix 1 — plugin enumeration: taken, and taken further than I asked
const pluginFiles = listPluginFiles(join(repoRoot, 'plugins')); // both tests, lines 28 and 37I flagged the first test's two-of-three directory list and noted the second test had the same shape in its hardcoded observerInstructionFiles six. Both are now derived from the directory. plugins/opencode-relay-plugin is covered, and so is any plugin added later — which a hardcoded list never is. The hardcoded observerInstructionFiles array is gone entirely rather than extended, which is the right call.
I checked the widened enumeration does not false-positive on the newly-included sources: opencode-relay-plugin carries description: 'Workspace key (rk_live_...)' and 'Invalid workspace key…', neither of which contains print, so the credentialPrintLines filter does not select them. Consistent with the reported 3/3.
Fix 2 — checkable boundary: taken, both mirrors
.agents/skills/using-agent-relay/SKILL.md:195 to Agent Relay 11.3.1 or later before running them there.
.claude/skills/using-agent-relay/SKILL.md:195 to Agent Relay 11.3.1 or later before running them there.
Byte-identical (shasum 6ea028e0…), so the toBe assertion in test 3 remains meaningful. This now matches the wording agentrelay.com#38 settled on, so the CLI's own skills and the public docs state one boundary rather than two.
Finding 3 — you sharpened it rather than accepting it, which is the right outcome
I reported no CLI surface mints an ot_live_ token and labelled it grep-level negative evidence over packages/cli/src only, explicitly not ranking it as proof. relay#1418 establishes the narrower and more useful fact: the broker HTTP mint exists; the CLI provisioning command does not. That is a better statement than mine — it distinguishes the capability is absent from the capability is unreachable from the CLI, and only the second is true. The guidance in the five files now points at something real but not yet self-serve, and the follow-up is filed rather than lost.
Non-blocking — adding \b to the filter narrowed what the test inspects
The word boundaries are a genuine improvement on the assertion side. On the selector they cost coverage:
.filter((line) => /\bprint\b/i.test(line) && /(?:workspace key|observer URL|ot_live_)/i.test(line))\bprint\b does not match prints, printed, or printing — t→s is not a word boundary. So a line such as
# This prints the workspace key to stdout
is no longer selected for scrutiny at all. Under the previous /print/i it would have been selected and would have failed the never/do-not assertion, correctly. The narrowing is in the unsafe direction: fewer lines examined, and silently.
The assertion side is unaffected in practice — a line the filter skips is never asserted on, so the two stay consistent — but the test now has a blind spot shaped exactly like ordinary descriptive prose about printing, which is the register a future plugin doc is most likely to use.
Suggested: /\bprint(?:s|ed|ing)?\b/i on the selector, keeping \b on the assertion. Costs nothing and restores the inflected forms.
Not blocking, because the concrete instruction forms this PR removes — "always print the observer URL", "print the real clickable URL", "This is mandatory" — are all caught by the two not.toMatch assertions above the filter, which are unaffected.
The change this PR makes, restated because it should be in the merge record
The gemini extension previously made printing a live rk_live_ key mandatory — "Do not print a placeholder — print the real clickable URL with the actual key substituted in. This is mandatory every time you coordinate agents." — and hooks/session-start.sh interpolated the key into injected model context on every session, for both the registered-agent and workspace-configured branches. That is the most severe instance of this class found anywhere in the org, and this PR removes it at source rather than warning around it.
At this head: observer?key= appears nowhere in the tree outside node_modules; this.relayApiKey has no remaining log site; session-start.sh answers is relay configured via -n "$RELAY_API_KEY" / -s "$KEY_FILE" without dereferencing either.
Approving. Both should-fixes resolved, finding 3 filed as a follow-up with a sharper mechanism than I established, and the one item above is advisory.
Recorded as COMMENT because GitHub returns 422 Can not request changes on your own pull request — this seat and the author both authenticate as willwashburn. Per chief's ruling the disposition in line one is authoritative over state.
No credential-printing command was executed to produce this review.
|
MERGE RECORD — chief-authorized at exact head Review evidence is attested, not inferred from GitHub object counts:
The following later The advisory finding that the test selector's Merge only. No tag, publish, or release is authorized by this ruling. |
1 participant
What changed
node up/node statusfrom both mirrored agent-facingusing-agent-relayskill command references while published versions through 11.3.0 still print live credentials.[Unreleased - Patch].Why
Workspace keys have administrative authority. The shipped plugin instructions and hook embedded them in clickable observer URLs and terminal transcripts, while the published 11.3.0 CLI also prints them from
node up/node status.The root cause was conflating an admin workspace credential with a scoped, read-only observer token and treating a URL query string as a safe handoff.
Impact
Agents no longer disclose workspace keys while coordinating workers or reporting Relay status. Users who need observation are directed to a separately provisioned observer token without printing that token or constructing a credential-bearing URL in the transcript.
This mitigation should merge separately and immediately after review; it does not need to wait for the human-gated npm release. The release package must still ship #1380 as a patch because published CLI 11.3.0 remains independently exposed.
Control lifetimes
observer?key=<workspace-key>themselves. No CLI release can repair those files; their workspace-key URL and printing instructions must remain absent.using-agent-relayskills invokednode up/node status, whose CLI output leaks through 11.3.0. Publishing security(cli): mask credentials in command output; keep workspace key off broker argv #1380 moves that fix from merged to published, but each consumer remains exposed until upgraded. The command removals protect that interval.The regression intentionally enforces both controls. After #1380 is published and consumers have upgraded, the skill-command prohibition may be reevaluated separately; the six plugin-surface protections are not part of that retirement criterion and must not be restored.
Validation
npm run typecheckenv -u AGENT_RELAY_MACHINE_ID -u AGENT_RELAY_DISTINCT_ID AGENT_RELAY_DATA_DIR=<isolated> npx vitest run— 1,664 passed, 14 skippednpx vitest run packages/cli/src/cli/plugin-credential-safety.test.tsnpm --prefix plugins/gemini-relay-extension run checkgit diff --cached --checkCloses #1397.