From 30a4cdc7419fce9f96f5869b71f9f302423af047 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 20 Oct 2026 18:00:45 -0700 Subject: [PATCH 1/5] Match session-handoff's Platform-Lane Exception to the Fix-and-Verify Rule (#3291) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary `session-handoff` "The Chain" states that a platform lane takes an issue only when no Linux host outside WSL can both fix the issue and verify the fix. The exception after it named verifying alone, and its "where" could be read as describing the backlog rather than as a condition. The exception now states the same condition as the rule: such a defect goes to the backlog when a Linux host outside WSL can both fix it and verify the fix. The path-translation example is its own sentence and names where the fix is made and tested, so it reads as one instance of that condition. The generated copies under `.claude-plugin/` and `.github/skills/` are regenerated with `scripts/build_dist.py`. Closes on promotion: #3275 ## Verification - `scripts/build_dist.py --check`: distributions current. - `scripts/prose_lint.py --diff origin/develop`, with and without `--check sentence-length`: clean. - markdownlint-cli2 over the three copies and `spec/validate.py`: clean. - Three local strict review passes. The first raised a sentence-length finding and the second raised an example that dropped the condition. Both are fixed on this branch, and the third pass raised none. 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Summary by CodeRabbit - **Documentation** - Updated platform-lane guidance so issues are routed to the ordinary backlog when a Linux host outside WSL can both fix and verify them. - Added a path-translation scenario as an example of this workflow. --- .agents/skills/session-handoff/SKILL.md | 5 +++-- .claude-plugin/fleet-skills/.source-digests/session-handoff | 2 +- .claude-plugin/fleet-skills/skills/session-handoff/SKILL.md | 5 +++-- .github/skills/session-handoff/SKILL.md | 5 +++-- 4 files changed, 10 insertions(+), 7 deletions(-) diff --git a/.agents/skills/session-handoff/SKILL.md b/.agents/skills/session-handoff/SKILL.md index 4264f53bf..655875368 100644 --- a/.agents/skills/session-handoff/SKILL.md +++ b/.agents/skills/session-handoff/SKILL.md @@ -70,8 +70,9 @@ without trusting the parts of it that have gone stale. session. A defect in WSL interop, the `WSLInterop` binfmt handler, `/mnt/` paths, or `wslpath` reproduces only inside WSL. So does running a Windows `.exe` from Linux, although WSL runs on Windows. Behavior that differs under Windows PowerShell 5.1 or `winget`, or in Windows - path and ACL semantics, reproduces only on Windows. Each of these defects still goes to the backlog where a Linux - host outside WSL verifies its fix, as a path-translation unit test can. An issue in + path and ACL semantics, reproduces only on Windows. Each of these defects still goes to the + backlog when a Linux host outside WSL can both fix it and verify the fix. A path-translation + fix made and unit-tested on a Linux host outside WSL is one such case. An issue in systemd user scopes, apt, a bash script, or a `pwsh` script run in a Linux container gets no lane by its topic. The conditions decide it like any other issue. Where only part of an issue needs a platform, the session finding it files each such part as its own issue for its platform's lane. diff --git a/.claude-plugin/fleet-skills/.source-digests/session-handoff b/.claude-plugin/fleet-skills/.source-digests/session-handoff index 341ebb874..3bbcc14cb 100644 --- a/.claude-plugin/fleet-skills/.source-digests/session-handoff +++ b/.claude-plugin/fleet-skills/.source-digests/session-handoff @@ -1 +1 @@ -1d8c4702f7b8a31c +b8bc9c5db7dab206 diff --git a/.claude-plugin/fleet-skills/skills/session-handoff/SKILL.md b/.claude-plugin/fleet-skills/skills/session-handoff/SKILL.md index 4264f53bf..655875368 100644 --- a/.claude-plugin/fleet-skills/skills/session-handoff/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/session-handoff/SKILL.md @@ -70,8 +70,9 @@ without trusting the parts of it that have gone stale. session. A defect in WSL interop, the `WSLInterop` binfmt handler, `/mnt/` paths, or `wslpath` reproduces only inside WSL. So does running a Windows `.exe` from Linux, although WSL runs on Windows. Behavior that differs under Windows PowerShell 5.1 or `winget`, or in Windows - path and ACL semantics, reproduces only on Windows. Each of these defects still goes to the backlog where a Linux - host outside WSL verifies its fix, as a path-translation unit test can. An issue in + path and ACL semantics, reproduces only on Windows. Each of these defects still goes to the + backlog when a Linux host outside WSL can both fix it and verify the fix. A path-translation + fix made and unit-tested on a Linux host outside WSL is one such case. An issue in systemd user scopes, apt, a bash script, or a `pwsh` script run in a Linux container gets no lane by its topic. The conditions decide it like any other issue. Where only part of an issue needs a platform, the session finding it files each such part as its own issue for its platform's lane. diff --git a/.github/skills/session-handoff/SKILL.md b/.github/skills/session-handoff/SKILL.md index 4264f53bf..655875368 100644 --- a/.github/skills/session-handoff/SKILL.md +++ b/.github/skills/session-handoff/SKILL.md @@ -70,8 +70,9 @@ without trusting the parts of it that have gone stale. session. A defect in WSL interop, the `WSLInterop` binfmt handler, `/mnt/` paths, or `wslpath` reproduces only inside WSL. So does running a Windows `.exe` from Linux, although WSL runs on Windows. Behavior that differs under Windows PowerShell 5.1 or `winget`, or in Windows - path and ACL semantics, reproduces only on Windows. Each of these defects still goes to the backlog where a Linux - host outside WSL verifies its fix, as a path-translation unit test can. An issue in + path and ACL semantics, reproduces only on Windows. Each of these defects still goes to the + backlog when a Linux host outside WSL can both fix it and verify the fix. A path-translation + fix made and unit-tested on a Linux host outside WSL is one such case. An issue in systemd user scopes, apt, a bash script, or a `pwsh` script run in a Linux container gets no lane by its topic. The conditions decide it like any other issue. Where only part of an issue needs a platform, the session finding it files each such part as its own issue for its platform's lane. From a86e92cd127bfff049e00d22b9b7e23a83e4b401 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 20 Oct 2026 18:21:45 -0700 Subject: [PATCH 2/5] Carry a Command the User Must Run Inside the Prompt's Own Question (#3293) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary `GOVERNANCE.md` "Communicating with the User" said where a clickable link goes when an interactive prompt cannot render it, and said nothing about a command the user has to paste and run. This adds a bullet beside the link bullet, which stays as it is: a question asking the user to run something carries the exact command in its own text, complete and ready to paste, and never points at an earlier message for it, since a prompt shows a command as plain text even though it cannot render a link. The section is included whole by the `agent-conduct` and `session-handoff` Skills, so their sources and generated distributions are regenerated with `scripts/build_dist.py`. ## Verification - `scripts/build_dist.py --check`, `scripts/prose_lint.py --diff` (with and without `--check sentence-length`), `spec/validate.py`, `scripts/repo_gate.py --check eol` and `--check eol-coverage`, and `scripts/docker_lint.py` (markdownlint, editorconfig-checker) all pass. - Three recorded `local-strict-review` passes at the `frontier` tier. The first raised three findings (two over-long sentences, an ambiguous pronoun, and wording that diverged from the issue), the second raised one (an unhedged claim that conflicted with the link bullet), and the third raised none. Each was fixed in its own commit. Closes on promotion: #3284 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- .agents/skills/agent-conduct/SKILL.md | 1 + .agents/skills/session-handoff/SKILL.md | 1 + .claude-plugin/fleet-skills/.source-digests/agent-conduct | 2 +- .claude-plugin/fleet-skills/.source-digests/session-handoff | 2 +- .claude-plugin/fleet-skills/skills/agent-conduct/SKILL.md | 1 + .claude-plugin/fleet-skills/skills/session-handoff/SKILL.md | 1 + .github/skills/agent-conduct/SKILL.md | 1 + .github/skills/session-handoff/SKILL.md | 1 + GOVERNANCE.md | 1 + 9 files changed, 9 insertions(+), 2 deletions(-) diff --git a/.agents/skills/agent-conduct/SKILL.md b/.agents/skills/agent-conduct/SKILL.md index aa93444c0..d5118c587 100644 --- a/.agents/skills/agent-conduct/SKILL.md +++ b/.agents/skills/agent-conduct/SKILL.md @@ -77,6 +77,7 @@ Claims about a pull request being reviewed, clean, or mergeable are owned by the - **Reference every pull request as a clickable link.** When you mention a PR on a surface that renders Markdown (chat, a summary, a report), render it as a Markdown link to the PR (`[#N](https://github.com/OWNER/REPO/pull/N)`), never a bare `#N`. The same applies to issues and commits. **The form follows the surface.** Some surfaces link neither a Markdown link nor a bare URL, an interactive prompt's question and option text among them, and pasting a full URL into one of those does not rescue it, since the reader gets a string to copy, which is the outcome this rule exists to prevent. There the reference is a bare `#N`, and the clickable link goes in the message that comes **before** the prompt rather than merely alongside it, because the prompt blocks on an answer and a message emitted after it is read once that answer is already given, which is the one moment the link is no longer any use. The test is whether the reader can click it where it is read, not whether it was written in the syntax that works elsewhere. +- **Put a command the user must run in the question's own text.** Where a question asks the user to run something, the question carries the exact command, complete and ready to paste. One example is a command the agent cannot run itself because the command prompts for a password. A message before the prompt may repeat the command, but the question never points at that message. A question pointing at it can leave the reader to find the command among a long run of earlier output. In the clickable-link bullet above, only the rendered link moves out of the prompt. A command needs no rendering, since the prompt shows it as plain text the reader can copy. - **Ask every question through the interface's prompt, never in prose.** When you need the user to decide, answer, or approve anything, put it to them through the interface's own prompt mechanism, whether or not work is blocked on it, and an offer to do more work ("want me to file that?") is a question like any other. A question written into a message is lost in the report around it however short it is, and one closing a long report sits where the reader is least likely to reach it. Where no prompt mechanism is available, the questions go as a numbered list opening the message rather than closing it, so the user can reply per number. This bullet settles how a question is put once it is asked, and whether a question is asked now or recorded for later is the parking bullet's to settle, the one opening "A question filed as an issue is parked rather than asked". - **Raise work blocked on the user as a direct interactive prompt.** When progress needs a decision, an authorization, or an answer only the user can give, ask for it through the interface's own prompt mechanism, at the point the work stops. Never leave it as prose in a summary: a handoff buried in a paragraph is a handoff that did not happen, because a summary reads as a report of finished work and the one line still waiting on the user is the easiest in it to skim past. The blocked item is the message, not a closing remark on a message about something else. **The options offered are the actions themselves**, and the one that unblocks the work names the action it authorizes ("squash and merge it"), so selecting it is the go-ahead rather than a note to act on later. Offering only ways to wait is the same failure in interactive clothing, since a prompt whose every choice is inaction reports the block rather than clearing it, and where the agent may not perform the authorized action itself, the option says who does it. Where no interactive prompt is available, the numbered list the bullet above names is the fallback. - **Lead every choice with a recommendation and its reason.** Wherever the user is asked to choose, in a prompt or in a numbered list, the option the agent recommends comes first and is marked as the recommendation, and every option states the reason for it. A user who takes the recommendation then reads one line, and one who does not sees what the other options trade away. A question with no defensible recommendation says so rather than inventing one. Where an open question has no options and the agent does have an answer, that answer and its reason go in the question's text, never as an invented option. diff --git a/.agents/skills/session-handoff/SKILL.md b/.agents/skills/session-handoff/SKILL.md index 655875368..530928be7 100644 --- a/.agents/skills/session-handoff/SKILL.md +++ b/.agents/skills/session-handoff/SKILL.md @@ -274,6 +274,7 @@ falls to the next session that has one. The rules below settle which case applie - **Reference every pull request as a clickable link.** When you mention a PR on a surface that renders Markdown (chat, a summary, a report), render it as a Markdown link to the PR (`[#N](https://github.com/OWNER/REPO/pull/N)`), never a bare `#N`. The same applies to issues and commits. **The form follows the surface.** Some surfaces link neither a Markdown link nor a bare URL, an interactive prompt's question and option text among them, and pasting a full URL into one of those does not rescue it, since the reader gets a string to copy, which is the outcome this rule exists to prevent. There the reference is a bare `#N`, and the clickable link goes in the message that comes **before** the prompt rather than merely alongside it, because the prompt blocks on an answer and a message emitted after it is read once that answer is already given, which is the one moment the link is no longer any use. The test is whether the reader can click it where it is read, not whether it was written in the syntax that works elsewhere. +- **Put a command the user must run in the question's own text.** Where a question asks the user to run something, the question carries the exact command, complete and ready to paste. One example is a command the agent cannot run itself because the command prompts for a password. A message before the prompt may repeat the command, but the question never points at that message. A question pointing at it can leave the reader to find the command among a long run of earlier output. In the clickable-link bullet above, only the rendered link moves out of the prompt. A command needs no rendering, since the prompt shows it as plain text the reader can copy. - **Ask every question through the interface's prompt, never in prose.** When you need the user to decide, answer, or approve anything, put it to them through the interface's own prompt mechanism, whether or not work is blocked on it, and an offer to do more work ("want me to file that?") is a question like any other. A question written into a message is lost in the report around it however short it is, and one closing a long report sits where the reader is least likely to reach it. Where no prompt mechanism is available, the questions go as a numbered list opening the message rather than closing it, so the user can reply per number. This bullet settles how a question is put once it is asked, and whether a question is asked now or recorded for later is the parking bullet's to settle, the one opening "A question filed as an issue is parked rather than asked". - **Raise work blocked on the user as a direct interactive prompt.** When progress needs a decision, an authorization, or an answer only the user can give, ask for it through the interface's own prompt mechanism, at the point the work stops. Never leave it as prose in a summary: a handoff buried in a paragraph is a handoff that did not happen, because a summary reads as a report of finished work and the one line still waiting on the user is the easiest in it to skim past. The blocked item is the message, not a closing remark on a message about something else. **The options offered are the actions themselves**, and the one that unblocks the work names the action it authorizes ("squash and merge it"), so selecting it is the go-ahead rather than a note to act on later. Offering only ways to wait is the same failure in interactive clothing, since a prompt whose every choice is inaction reports the block rather than clearing it, and where the agent may not perform the authorized action itself, the option says who does it. Where no interactive prompt is available, the numbered list the bullet above names is the fallback. - **Lead every choice with a recommendation and its reason.** Wherever the user is asked to choose, in a prompt or in a numbered list, the option the agent recommends comes first and is marked as the recommendation, and every option states the reason for it. A user who takes the recommendation then reads one line, and one who does not sees what the other options trade away. A question with no defensible recommendation says so rather than inventing one. Where an open question has no options and the agent does have an answer, that answer and its reason go in the question's text, never as an invented option. diff --git a/.claude-plugin/fleet-skills/.source-digests/agent-conduct b/.claude-plugin/fleet-skills/.source-digests/agent-conduct index 5e7d9b244..cded7e77e 100644 --- a/.claude-plugin/fleet-skills/.source-digests/agent-conduct +++ b/.claude-plugin/fleet-skills/.source-digests/agent-conduct @@ -1 +1 @@ -104c9334429bf721 +af896a6f3797483f diff --git a/.claude-plugin/fleet-skills/.source-digests/session-handoff b/.claude-plugin/fleet-skills/.source-digests/session-handoff index 3bbcc14cb..4d33b2a9a 100644 --- a/.claude-plugin/fleet-skills/.source-digests/session-handoff +++ b/.claude-plugin/fleet-skills/.source-digests/session-handoff @@ -1 +1 @@ -b8bc9c5db7dab206 +9438d88bc6983156 diff --git a/.claude-plugin/fleet-skills/skills/agent-conduct/SKILL.md b/.claude-plugin/fleet-skills/skills/agent-conduct/SKILL.md index aa93444c0..d5118c587 100644 --- a/.claude-plugin/fleet-skills/skills/agent-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/agent-conduct/SKILL.md @@ -77,6 +77,7 @@ Claims about a pull request being reviewed, clean, or mergeable are owned by the - **Reference every pull request as a clickable link.** When you mention a PR on a surface that renders Markdown (chat, a summary, a report), render it as a Markdown link to the PR (`[#N](https://github.com/OWNER/REPO/pull/N)`), never a bare `#N`. The same applies to issues and commits. **The form follows the surface.** Some surfaces link neither a Markdown link nor a bare URL, an interactive prompt's question and option text among them, and pasting a full URL into one of those does not rescue it, since the reader gets a string to copy, which is the outcome this rule exists to prevent. There the reference is a bare `#N`, and the clickable link goes in the message that comes **before** the prompt rather than merely alongside it, because the prompt blocks on an answer and a message emitted after it is read once that answer is already given, which is the one moment the link is no longer any use. The test is whether the reader can click it where it is read, not whether it was written in the syntax that works elsewhere. +- **Put a command the user must run in the question's own text.** Where a question asks the user to run something, the question carries the exact command, complete and ready to paste. One example is a command the agent cannot run itself because the command prompts for a password. A message before the prompt may repeat the command, but the question never points at that message. A question pointing at it can leave the reader to find the command among a long run of earlier output. In the clickable-link bullet above, only the rendered link moves out of the prompt. A command needs no rendering, since the prompt shows it as plain text the reader can copy. - **Ask every question through the interface's prompt, never in prose.** When you need the user to decide, answer, or approve anything, put it to them through the interface's own prompt mechanism, whether or not work is blocked on it, and an offer to do more work ("want me to file that?") is a question like any other. A question written into a message is lost in the report around it however short it is, and one closing a long report sits where the reader is least likely to reach it. Where no prompt mechanism is available, the questions go as a numbered list opening the message rather than closing it, so the user can reply per number. This bullet settles how a question is put once it is asked, and whether a question is asked now or recorded for later is the parking bullet's to settle, the one opening "A question filed as an issue is parked rather than asked". - **Raise work blocked on the user as a direct interactive prompt.** When progress needs a decision, an authorization, or an answer only the user can give, ask for it through the interface's own prompt mechanism, at the point the work stops. Never leave it as prose in a summary: a handoff buried in a paragraph is a handoff that did not happen, because a summary reads as a report of finished work and the one line still waiting on the user is the easiest in it to skim past. The blocked item is the message, not a closing remark on a message about something else. **The options offered are the actions themselves**, and the one that unblocks the work names the action it authorizes ("squash and merge it"), so selecting it is the go-ahead rather than a note to act on later. Offering only ways to wait is the same failure in interactive clothing, since a prompt whose every choice is inaction reports the block rather than clearing it, and where the agent may not perform the authorized action itself, the option says who does it. Where no interactive prompt is available, the numbered list the bullet above names is the fallback. - **Lead every choice with a recommendation and its reason.** Wherever the user is asked to choose, in a prompt or in a numbered list, the option the agent recommends comes first and is marked as the recommendation, and every option states the reason for it. A user who takes the recommendation then reads one line, and one who does not sees what the other options trade away. A question with no defensible recommendation says so rather than inventing one. Where an open question has no options and the agent does have an answer, that answer and its reason go in the question's text, never as an invented option. diff --git a/.claude-plugin/fleet-skills/skills/session-handoff/SKILL.md b/.claude-plugin/fleet-skills/skills/session-handoff/SKILL.md index 655875368..530928be7 100644 --- a/.claude-plugin/fleet-skills/skills/session-handoff/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/session-handoff/SKILL.md @@ -274,6 +274,7 @@ falls to the next session that has one. The rules below settle which case applie - **Reference every pull request as a clickable link.** When you mention a PR on a surface that renders Markdown (chat, a summary, a report), render it as a Markdown link to the PR (`[#N](https://github.com/OWNER/REPO/pull/N)`), never a bare `#N`. The same applies to issues and commits. **The form follows the surface.** Some surfaces link neither a Markdown link nor a bare URL, an interactive prompt's question and option text among them, and pasting a full URL into one of those does not rescue it, since the reader gets a string to copy, which is the outcome this rule exists to prevent. There the reference is a bare `#N`, and the clickable link goes in the message that comes **before** the prompt rather than merely alongside it, because the prompt blocks on an answer and a message emitted after it is read once that answer is already given, which is the one moment the link is no longer any use. The test is whether the reader can click it where it is read, not whether it was written in the syntax that works elsewhere. +- **Put a command the user must run in the question's own text.** Where a question asks the user to run something, the question carries the exact command, complete and ready to paste. One example is a command the agent cannot run itself because the command prompts for a password. A message before the prompt may repeat the command, but the question never points at that message. A question pointing at it can leave the reader to find the command among a long run of earlier output. In the clickable-link bullet above, only the rendered link moves out of the prompt. A command needs no rendering, since the prompt shows it as plain text the reader can copy. - **Ask every question through the interface's prompt, never in prose.** When you need the user to decide, answer, or approve anything, put it to them through the interface's own prompt mechanism, whether or not work is blocked on it, and an offer to do more work ("want me to file that?") is a question like any other. A question written into a message is lost in the report around it however short it is, and one closing a long report sits where the reader is least likely to reach it. Where no prompt mechanism is available, the questions go as a numbered list opening the message rather than closing it, so the user can reply per number. This bullet settles how a question is put once it is asked, and whether a question is asked now or recorded for later is the parking bullet's to settle, the one opening "A question filed as an issue is parked rather than asked". - **Raise work blocked on the user as a direct interactive prompt.** When progress needs a decision, an authorization, or an answer only the user can give, ask for it through the interface's own prompt mechanism, at the point the work stops. Never leave it as prose in a summary: a handoff buried in a paragraph is a handoff that did not happen, because a summary reads as a report of finished work and the one line still waiting on the user is the easiest in it to skim past. The blocked item is the message, not a closing remark on a message about something else. **The options offered are the actions themselves**, and the one that unblocks the work names the action it authorizes ("squash and merge it"), so selecting it is the go-ahead rather than a note to act on later. Offering only ways to wait is the same failure in interactive clothing, since a prompt whose every choice is inaction reports the block rather than clearing it, and where the agent may not perform the authorized action itself, the option says who does it. Where no interactive prompt is available, the numbered list the bullet above names is the fallback. - **Lead every choice with a recommendation and its reason.** Wherever the user is asked to choose, in a prompt or in a numbered list, the option the agent recommends comes first and is marked as the recommendation, and every option states the reason for it. A user who takes the recommendation then reads one line, and one who does not sees what the other options trade away. A question with no defensible recommendation says so rather than inventing one. Where an open question has no options and the agent does have an answer, that answer and its reason go in the question's text, never as an invented option. diff --git a/.github/skills/agent-conduct/SKILL.md b/.github/skills/agent-conduct/SKILL.md index aa93444c0..d5118c587 100644 --- a/.github/skills/agent-conduct/SKILL.md +++ b/.github/skills/agent-conduct/SKILL.md @@ -77,6 +77,7 @@ Claims about a pull request being reviewed, clean, or mergeable are owned by the - **Reference every pull request as a clickable link.** When you mention a PR on a surface that renders Markdown (chat, a summary, a report), render it as a Markdown link to the PR (`[#N](https://github.com/OWNER/REPO/pull/N)`), never a bare `#N`. The same applies to issues and commits. **The form follows the surface.** Some surfaces link neither a Markdown link nor a bare URL, an interactive prompt's question and option text among them, and pasting a full URL into one of those does not rescue it, since the reader gets a string to copy, which is the outcome this rule exists to prevent. There the reference is a bare `#N`, and the clickable link goes in the message that comes **before** the prompt rather than merely alongside it, because the prompt blocks on an answer and a message emitted after it is read once that answer is already given, which is the one moment the link is no longer any use. The test is whether the reader can click it where it is read, not whether it was written in the syntax that works elsewhere. +- **Put a command the user must run in the question's own text.** Where a question asks the user to run something, the question carries the exact command, complete and ready to paste. One example is a command the agent cannot run itself because the command prompts for a password. A message before the prompt may repeat the command, but the question never points at that message. A question pointing at it can leave the reader to find the command among a long run of earlier output. In the clickable-link bullet above, only the rendered link moves out of the prompt. A command needs no rendering, since the prompt shows it as plain text the reader can copy. - **Ask every question through the interface's prompt, never in prose.** When you need the user to decide, answer, or approve anything, put it to them through the interface's own prompt mechanism, whether or not work is blocked on it, and an offer to do more work ("want me to file that?") is a question like any other. A question written into a message is lost in the report around it however short it is, and one closing a long report sits where the reader is least likely to reach it. Where no prompt mechanism is available, the questions go as a numbered list opening the message rather than closing it, so the user can reply per number. This bullet settles how a question is put once it is asked, and whether a question is asked now or recorded for later is the parking bullet's to settle, the one opening "A question filed as an issue is parked rather than asked". - **Raise work blocked on the user as a direct interactive prompt.** When progress needs a decision, an authorization, or an answer only the user can give, ask for it through the interface's own prompt mechanism, at the point the work stops. Never leave it as prose in a summary: a handoff buried in a paragraph is a handoff that did not happen, because a summary reads as a report of finished work and the one line still waiting on the user is the easiest in it to skim past. The blocked item is the message, not a closing remark on a message about something else. **The options offered are the actions themselves**, and the one that unblocks the work names the action it authorizes ("squash and merge it"), so selecting it is the go-ahead rather than a note to act on later. Offering only ways to wait is the same failure in interactive clothing, since a prompt whose every choice is inaction reports the block rather than clearing it, and where the agent may not perform the authorized action itself, the option says who does it. Where no interactive prompt is available, the numbered list the bullet above names is the fallback. - **Lead every choice with a recommendation and its reason.** Wherever the user is asked to choose, in a prompt or in a numbered list, the option the agent recommends comes first and is marked as the recommendation, and every option states the reason for it. A user who takes the recommendation then reads one line, and one who does not sees what the other options trade away. A question with no defensible recommendation says so rather than inventing one. Where an open question has no options and the agent does have an answer, that answer and its reason go in the question's text, never as an invented option. diff --git a/.github/skills/session-handoff/SKILL.md b/.github/skills/session-handoff/SKILL.md index 655875368..530928be7 100644 --- a/.github/skills/session-handoff/SKILL.md +++ b/.github/skills/session-handoff/SKILL.md @@ -274,6 +274,7 @@ falls to the next session that has one. The rules below settle which case applie - **Reference every pull request as a clickable link.** When you mention a PR on a surface that renders Markdown (chat, a summary, a report), render it as a Markdown link to the PR (`[#N](https://github.com/OWNER/REPO/pull/N)`), never a bare `#N`. The same applies to issues and commits. **The form follows the surface.** Some surfaces link neither a Markdown link nor a bare URL, an interactive prompt's question and option text among them, and pasting a full URL into one of those does not rescue it, since the reader gets a string to copy, which is the outcome this rule exists to prevent. There the reference is a bare `#N`, and the clickable link goes in the message that comes **before** the prompt rather than merely alongside it, because the prompt blocks on an answer and a message emitted after it is read once that answer is already given, which is the one moment the link is no longer any use. The test is whether the reader can click it where it is read, not whether it was written in the syntax that works elsewhere. +- **Put a command the user must run in the question's own text.** Where a question asks the user to run something, the question carries the exact command, complete and ready to paste. One example is a command the agent cannot run itself because the command prompts for a password. A message before the prompt may repeat the command, but the question never points at that message. A question pointing at it can leave the reader to find the command among a long run of earlier output. In the clickable-link bullet above, only the rendered link moves out of the prompt. A command needs no rendering, since the prompt shows it as plain text the reader can copy. - **Ask every question through the interface's prompt, never in prose.** When you need the user to decide, answer, or approve anything, put it to them through the interface's own prompt mechanism, whether or not work is blocked on it, and an offer to do more work ("want me to file that?") is a question like any other. A question written into a message is lost in the report around it however short it is, and one closing a long report sits where the reader is least likely to reach it. Where no prompt mechanism is available, the questions go as a numbered list opening the message rather than closing it, so the user can reply per number. This bullet settles how a question is put once it is asked, and whether a question is asked now or recorded for later is the parking bullet's to settle, the one opening "A question filed as an issue is parked rather than asked". - **Raise work blocked on the user as a direct interactive prompt.** When progress needs a decision, an authorization, or an answer only the user can give, ask for it through the interface's own prompt mechanism, at the point the work stops. Never leave it as prose in a summary: a handoff buried in a paragraph is a handoff that did not happen, because a summary reads as a report of finished work and the one line still waiting on the user is the easiest in it to skim past. The blocked item is the message, not a closing remark on a message about something else. **The options offered are the actions themselves**, and the one that unblocks the work names the action it authorizes ("squash and merge it"), so selecting it is the go-ahead rather than a note to act on later. Offering only ways to wait is the same failure in interactive clothing, since a prompt whose every choice is inaction reports the block rather than clearing it, and where the agent may not perform the authorized action itself, the option says who does it. Where no interactive prompt is available, the numbered list the bullet above names is the fallback. - **Lead every choice with a recommendation and its reason.** Wherever the user is asked to choose, in a prompt or in a numbered list, the option the agent recommends comes first and is marked as the recommendation, and every option states the reason for it. A user who takes the recommendation then reads one line, and one who does not sees what the other options trade away. A question with no defensible recommendation says so rather than inventing one. Where an open question has no options and the agent does have an answer, that answer and its reason go in the question's text, never as an invented option. diff --git a/GOVERNANCE.md b/GOVERNANCE.md index df57d5fa0..249ac6f95 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -251,6 +251,7 @@ The provider-specific mechanics this contract needs to actually drive GitHub Cop ## Communicating with the User - **Reference every pull request as a clickable link.** When you mention a PR on a surface that renders Markdown (chat, a summary, a report), render it as a Markdown link to the PR (`[#N](https://github.com/OWNER/REPO/pull/N)`), never a bare `#N`. The same applies to issues and commits. **The form follows the surface.** Some surfaces link neither a Markdown link nor a bare URL, an interactive prompt's question and option text among them, and pasting a full URL into one of those does not rescue it, since the reader gets a string to copy, which is the outcome this rule exists to prevent. There the reference is a bare `#N`, and the clickable link goes in the message that comes **before** the prompt rather than merely alongside it, because the prompt blocks on an answer and a message emitted after it is read once that answer is already given, which is the one moment the link is no longer any use. The test is whether the reader can click it where it is read, not whether it was written in the syntax that works elsewhere. +- **Put a command the user must run in the question's own text.** Where a question asks the user to run something, the question carries the exact command, complete and ready to paste. One example is a command the agent cannot run itself because the command prompts for a password. A message before the prompt may repeat the command, but the question never points at that message. A question pointing at it can leave the reader to find the command among a long run of earlier output. In the clickable-link bullet above, only the rendered link moves out of the prompt. A command needs no rendering, since the prompt shows it as plain text the reader can copy. - **Ask every question through the interface's prompt, never in prose.** When you need the user to decide, answer, or approve anything, put it to them through the interface's own prompt mechanism, whether or not work is blocked on it, and an offer to do more work ("want me to file that?") is a question like any other. A question written into a message is lost in the report around it however short it is, and one closing a long report sits where the reader is least likely to reach it. Where no prompt mechanism is available, the questions go as a numbered list opening the message rather than closing it, so the user can reply per number. This bullet settles how a question is put once it is asked, and whether a question is asked now or recorded for later is the parking bullet's to settle, the one opening "A question filed as an issue is parked rather than asked". - **Raise work blocked on the user as a direct interactive prompt.** When progress needs a decision, an authorization, or an answer only the user can give, ask for it through the interface's own prompt mechanism, at the point the work stops. Never leave it as prose in a summary: a handoff buried in a paragraph is a handoff that did not happen, because a summary reads as a report of finished work and the one line still waiting on the user is the easiest in it to skim past. The blocked item is the message, not a closing remark on a message about something else. **The options offered are the actions themselves**, and the one that unblocks the work names the action it authorizes ("squash and merge it"), so selecting it is the go-ahead rather than a note to act on later. Offering only ways to wait is the same failure in interactive clothing, since a prompt whose every choice is inaction reports the block rather than clearing it, and where the agent may not perform the authorized action itself, the option says who does it. Where no interactive prompt is available, the numbered list the bullet above names is the fallback. - **Lead every choice with a recommendation and its reason.** Wherever the user is asked to choose, in a prompt or in a numbered list, the option the agent recommends comes first and is marked as the recommendation, and every option states the reason for it. A user who takes the recommendation then reads one line, and one who does not sees what the other options trade away. A question with no defensible recommendation says so rather than inventing one. Where an open question has no options and the agent does have an answer, that answer and its reason go in the question's text, never as an invented option. From b3637456fe531831cc4ccf76717095097e359e16 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 20 Oct 2026 18:58:36 -0700 Subject: [PATCH 3/5] Correct the Smoke Stub's enable_* Description in the Caller-Stub Paragraph (#3295) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #3282 The caller-stub paragraph in docs/reusable-workflows.md said the smoke job forwards the paths filter's enable_* outputs. The catalog smoke stub passes its own fixed enable_* set, the same one its publish-release.yml passes, and its `changes` paths filter only decides whether the smoke job runs. The dispatch-bypass note now reads as guidance for a multi-target caller that wires filter outputs into enable_* inputs. WORKFLOW.md mentions a smoke-build enable-forward in several places and is not changed here. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Sonnet 5.5 --- docs/reusable-workflows.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/reusable-workflows.md b/docs/reusable-workflows.md index 6ba3fdf4e..63dab08be 100644 --- a/docs/reusable-workflows.md +++ b/docs/reusable-workflows.md @@ -507,7 +507,7 @@ The `get-version` example passes no `ref`, so the task versions the triggering c ## Adopting the Release Chain -A downstream repo replaces its carried release orchestrator and per-target leaf tasks with a caller stub in its own `publish-release.yml` reaching the hub tasks by pin. `test-pull-request.yml`'s smoke job calls `build-release-task.yml` the same way, with `smoke: true` and the paths-filter's `enable_*` outputs. A dispatch bypass on a caller that maps filter outputs to `enable_*` inputs must also force them on. A dispatch run has no pull request diff, so the outputs can leave every target disabled. The full shape below is the catalog snippet [`publish-release.yml`][release-caller-snippet] byte for byte, pinned to a release carrying the current task interface, and Dependabot bumps it from there ([Pinning][pinning]). The task declares no job-level `permissions:` of its own, because a called job's block is validated against the caller's grant before its `if:` runs and would fail a caller that does not grant it at startup. The caller therefore grants only what its enabled paths write with: `contents: write` and `actions: write` when it sets `github: true` on a non-smoke run (the release upload, the artifact cleanup, and the [`WORKFLOW.md`][workflow] D4.7 publisher dispatch and self-cancel), and nothing beyond `contents: read` on a build-only or smoke run, where a Dependabot pull request holds a read-only token. No call to this task ever needs `id-token: write`, because neither package push happens inside it, for the reason the next paragraph gives. +A downstream repo replaces its carried release orchestrator and per-target leaf tasks with a caller stub in its own `publish-release.yml` reaching the hub tasks by pin. `test-pull-request.yml`'s smoke job calls `build-release-task.yml` the same way, with `smoke: true` and its own fixed `enable_*` set, the same one its `publish-release.yml` passes. The `changes` paths filter in `test-pull-request.yml` decides only whether the smoke job runs. A multi-target caller that wires filter outputs into `enable_*` inputs and adds a dispatch bypass must also force them on. A dispatch run has no pull request diff, so the outputs can leave every target disabled. The full shape below is the catalog snippet [`publish-release.yml`][release-caller-snippet] byte for byte, pinned to a release carrying the current task interface, and Dependabot bumps it from there ([Pinning][pinning]). The task declares no job-level `permissions:` of its own, because a called job's block is validated against the caller's grant before its `if:` runs and would fail a caller that does not grant it at startup. The caller therefore grants only what its enabled paths write with: `contents: write` and `actions: write` when it sets `github: true` on a non-smoke run (the release upload, the artifact cleanup, and the [`WORKFLOW.md`][workflow] D4.7 publisher dispatch and self-cancel), and nothing beyond `contents: read` on a build-only or smoke run, where a Dependabot pull request holds a read-only token. No call to this task ever needs `id-token: write`, because neither package push happens inside it, for the reason the next paragraph gives. Neither package push runs inside the hub task, and that is a constraint rather than a preference. NuGet.org and PyPI trusted publishing both validate the OIDC token's `job_workflow_ref` claim against the repository that owns the package, and that claim names the workflow file the job actually ran from. A job running from a hub task therefore carries `ptr727/ProjectTemplate/.github/workflows/build-release-task.yml@`, and the token exchange is rejected, NuGet.org answering `HTTP 401` with `does not start with //.github/workflows/` and PyPI rejecting the same shape under its own code. A caller hook does not avoid it, since a composite action runs inside the hub's job and leaves the claim unchanged. The hub task instead builds the package and uploads it as `nuget-build-` or `pypi-build-`, and the caller stub's own `publish-nuget` or `publish-pypi` job downloads that artifact and pushes, so the claim names the publishing repository. A smoke build never reaches either push, which is why a pull request cannot catch this and the first real release is where it surfaces. From 390e2250bc55b808a2e18499a970392535d23b1f Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 20 Oct 2026 19:15:03 -0700 Subject: [PATCH 4/5] Fail Closed on an Unread Write Target With No Origin (#3297) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary With no resolvable origin, the write guard now fails closed on a gh write whose target it does not read. That covers a gh write an `eval` runs, a `gh api` write to a `repositories/` endpoint, and a `gh api` write through a `..` segment. A literal target keeps the no-origin allow, since there is no owner to compare it against. The run-time-word check and the cross-owner comparison stay inside the origin branch. ## Changes - `classify()` in `host-setup/agent-safety/claude/gh-write-guard.py` calls `_gh_unread_target` after the `if origin:` block rather than inside it. With an origin resolved, the check order and every decision are unchanged. The comment above the block now states which checks need an origin. - A new `_NO_ORIGIN_CASES` table and `_no_origin_selftest()` run with `_origin_owner_repo` pinned to resolve nothing. The pin is restored in a `finally`. The table holds deny cases for each unread shape and allow cases for a literal cross-owner target, a write naming no target, a run-time `--repo`, and reads. - `host-setup/agent-safety/README.md` requirement 3 and the "Opaque targets are unseen" bullet in `host-setup/agent-safety/claude/README.md` now state the no-origin posture: the owner comparison and the run-time-word check are skipped, and an unread target still fails closed. ## Scope Note for the Maintainer `_gh_unread_target` also covers a write taking its repository from a `GH_REPO` that an unread builtin sets, such as `declare -x`, `read`, `set -a`, an `eval`, or `GH_REPO+=`. That shape moves with the call. So outside a checkout, `declare -x GH_REPO=stranger/example; gh pr comment 5 -b x` is now denied, though the value is written out. This fits "an unread target fails closed", since the hook does not read those builtins. It may differ from what "literal targets keep the no-origin allow" meant. The self-test pins the deny so either answer is a one-line change. ## Verification - `python3 host-setup/agent-safety/claude/gh-write-guard.py --selftest`: SELFTEST PASS, including every new no-origin case. - `python3 host-setup/agent-safety/claude/test_install.py`: OK. - Revert proof: a scratch copy of this branch's guard with only the code move undone (tests kept) fails exactly the four no-origin deny cases (`eval`, `repositories/`, `..` segment, `declare -x GH_REPO`), each read as allow. The working tree was never reverted. - Develop-versus-branch differential: develop's guard, loaded from `git show origin/develop:...` into a fresh `mktemp -d`, and this branch's guard classified the same constructed corpus under `timeout`. The corpus is every command in the guard's `*_CASES` tables plus the no-origin cases, classified with two resolved origins and with no origin. Every flip is allow to deny, with no origin, through the unread-target deny message. There are zero deny-to-allow flips and zero flips with an origin resolved. The flipped forms are the `eval` write, the `repositories/` endpoint, the `..` segment, and the unread-builtin `GH_REPO` shapes. - Gates: `prose_lint.py --diff origin/develop` (with `--check sentence-length` too), `repo_gate.py --check eol` and `eol-coverage`, `docker_lint.py`, `spec/validate.py`, and `ruff check` plus `ruff format --check`. - Local strict review: three recorded passes. The first found sentence-length lines, a closed rule list on line 158 of the claude README, and an ambiguous pronoun, all fixed. The second found an unreflowed docstring and a comment narrower than the code, both fixed. The third found nothing. The comment lines this change adds need the `comments` label. Closes #3288 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- host-setup/agent-safety/README.md | 10 ++- host-setup/agent-safety/claude/README.md | 2 +- .../agent-safety/claude/gh-write-guard.py | 82 +++++++++++++++++-- 3 files changed, 82 insertions(+), 12 deletions(-) diff --git a/host-setup/agent-safety/README.md b/host-setup/agent-safety/README.md index b02ab6164..50d58b7d3 100644 --- a/host-setup/agent-safety/README.md +++ b/host-setup/agent-safety/README.md @@ -49,10 +49,12 @@ text says, because the harm it covers was never in the text. allowed only when named in a grant read from the environment the session was launched with -- never a channel the agent itself can set (an inline `VAR=x cmd` prefix or an `export` inside the same call must not satisfy this). **When no `origin` resolves at all** (a non-git directory, or a - checkout whose remote can't be read), this requirement has nothing to compare the target against - and does not fire -- requirements 1 and 2 still apply regardless, and this is the same - precision-over-recall stance every requirement but 4 takes. A write is recognized by its command - and subcommand as gh's own parser finds them, past the flags before each. So a repo flag before + checkout whose remote can't be read), no owner exists to compare against. This requirement's + owner comparison and its run-time-word check then do not fire, while requirements 1 and 2 still + apply. This is the same precision-over-recall stance every requirement but 4 takes. A write whose + target the hook does not read, described below, fails closed with no origin too, since that + target is unknown either way. A write is recognized by its command and subcommand as gh's own + parser finds them, past the flags before each. So a repo flag before the command or between `pr` and its subcommand still names the target. The target is also the `R` in a short-option cluster such as `-dR`, and a `GH_REPO` value that reaches the gh word, the one in the environment the session was launched with included. A repository named by a diff --git a/host-setup/agent-safety/claude/README.md b/host-setup/agent-safety/claude/README.md index d11c12033..c2cf0b0f7 100644 --- a/host-setup/agent-safety/claude/README.md +++ b/host-setup/agent-safety/claude/README.md @@ -155,7 +155,7 @@ Every other key in the file is left as it stands, `permissions.allow` and the re - **Per-machine.** `~/.claude/` does not travel, so run the installer on each box. This is the rollout that [#365][issue-365] tracks. - **Precision over recall for the write footguns.** The hook denies the specific dangerous write shapes with high confidence rather than gating every write, so it is written to avoid blocking legitimate work -- a known false-positive shape is a parser defect to fix, not an accepted cost. The exception is a write whose target the hook cannot compare, which it denies. That is a write whose repository owner is a run-time word. It is also one whose target the hook does not read, as the root spec's requirement 3 lists. A shape it does not catch still falls under the behavioral rules. - **The branch-bypass rule fails closed.** Unlike the write-footgun rules, a push to `main`/`master`/`develop` is denied even when its rules cannot be determined (the API is unreachable, or the checkout's origin cannot be resolved to query them), because the harm there is a silent success under the maintainer's admin bypass. The rule reads each branch's live rules, so it adapts to every repo (a code-style `develop` denies, a config-style `develop` allows) with no per-repo configuration, and hands the exact command to the maintainer to run when a bypass is genuinely intended. -- **Opaque targets are unseen.** The hook cannot see the repository behind a GraphQL node id, which is exactly why rule 2 blocks a *literal* id at all, since a captured `$variable` is trusted. Likewise, the cross-origin check only runs when an `origin` can be resolved and the write names an explicit target, such as `-R`, `repos//`, a `GH_REPO` value, or a pull request URL. A write from a non-git directory, or one whose target is only a node id, is evaluated by rules 1 and 2 alone. The hook reads an inherited `GH_REPO` from its own environment, the one the session was launched with. A value a Bash tool call gets some other way, such as from a shell profile it sources, is not seen. An inherited value is read beside any `export` in the command, since the hook cannot tell whether that export runs. To write elsewhere, override it with a prefix, as in `GH_REPO=/ gh ...`, or with `-R`. Where a command-text `export` may not run where it sits, as in a subshell or a branch, every value the command sets is read. A `gh api` endpoint's run-time segment past a literal owner is compared by that owner alone. Such a segment may still hold a `..` segment that reaches another owner. +- **Opaque targets are unseen.** The hook cannot see the repository behind a GraphQL node id. That is why rule 2 blocks a *literal* id at all, since a captured `$variable` is trusted. Likewise, the cross-origin check only runs when an `origin` can be resolved and the write names an explicit target. Such a target is `-R`, `repos//`, a `GH_REPO` value, or a pull request URL. No owner comparison reads a write whose target is only a node id. A write from a non-git directory skips only the owner comparison and the run-time-word check. A target the hook does not read fails closed outside a checkout too. Examples are a gh write an `eval` runs and a `gh api` write to a `repositories/` endpoint or through a `..` segment. The hook reads an inherited `GH_REPO` from its own environment, the one the session was launched with. A value a Bash tool call gets some other way, such as from a shell profile it sources, is not seen. An inherited value is read beside any `export` in the command, since the hook cannot tell whether that export runs. To write elsewhere, override it with a prefix, as in `GH_REPO=/ gh ...`, or with `-R`. Where a command-text `export` may not run where it sits, as in a subshell or a branch, every value the command sets is read. A `gh api` endpoint's run-time segment past a literal owner is compared by that owner alone. Such a segment may still hold a `..` segment that reaches another owner. - **A write or a wait inside a script file is unseen, so never put either in a script to run it.** The hook reads the command the agent runs, which for `bash deploy.sh` is that one string. A `git push` or a `gh` mutation inside the script therefore reaches the server unparsed. An unbounded wait inside that same script is equally unseen, and worse in one way. A denied write fails loudly. A wait the guard never saw runs until something ends it, which is one of the leaks the SessionEnd sweep exists to find afterwards. This is the widest hole in the kit, and an agent opens it by accident. Collecting fifteen repetitive pushes into a loop looks like tidiness, and so does moving a poll into a script to keep a tool call short. Neither looks like disabling a guard. Issue each write and each wait as its own command. A script that only reads, computes, or prepares local commits is fine, because the boundary is the write and the wait rather than the script. - **Not a credential control.** A fine-grained PAT limited to owned repositories is a separate, stronger structural guard (a hard `403` on any non-owned repo) and is left to per-machine credential setup, out of this kit. - **Rule 6's ref-checkout and `--ff-only` exemptions are deliberate, not a gap.** The #1073 incident's own literal commands (an unforced `git checkout main`, then `git pull --ff-only`) are exactly this shape, so this rule does not deny the incident's own commands specifically. Denying them would also break the documented base-clone cleanup step in the `repo-worktree` skill, which needs precisely those two. The concurrent-access hazard those commands still carry (switching HEAD, or fast-forwarding, in a checkout another task might be relying on) is not decidable from the command text alone, so it stays the prose rule's job. A non-force flag alongside the ref, such as `--detach`, is exempt too, not only a strictly zero-flags invocation -- git's own overwrite-refusal on a dirty tracked file is unaffected by it, verified live. diff --git a/host-setup/agent-safety/claude/gh-write-guard.py b/host-setup/agent-safety/claude/gh-write-guard.py index 415a3ce2f..769a7d27d 100755 --- a/host-setup/agent-safety/claude/gh-write-guard.py +++ b/host-setup/agent-safety/claude/gh-write-guard.py @@ -7406,7 +7406,8 @@ def classify( # `_gh_write_targets` reads argv position within each real `gh` invocation, so a compound command carrying one target per invocation still has every one read (the write after `&&` is not skipped). # A --repo/repos// mention living inside an unrelated --body/--title/-f value, or in a non-gh command entirely, is not read as a target. targets = _gh_write_targets(cmd, environ) - # This only runs when origin resolves, meaning a git checkout, since with no project context there is nothing to compare an explicit target against, so the check is skipped and rules 1 and 2 still apply. + # The owner comparison and the run-time-word check run only when origin resolves, since with no origin there is no owner to compare against. + # The unread-target check after this block runs either way, since a target the guard does not read is unknown with or without an origin. # A node-id target is invisible here regardless, which is what rule 2 guards. if origin: granted = _granted_targets(environ) @@ -7431,12 +7432,12 @@ def classify( "it writes to. Failing closed. Write the repository out, as in --repo owner/repo, " "GH_REPO=owner/repo, or repos/owner/repo." + _handoff(cmd) ) - unread = _gh_unread_target(cmd, payloads) - if unread: - return "deny", ( - f"This gh write {unread[0]}, so the guard cannot tell which repository it writes " - f"to. Failing closed. {unread[1]}" + _handoff(cmd) - ) + unread = _gh_unread_target(cmd, payloads) + if unread: + return "deny", ( + f"This gh write {unread[0]}, so the guard cannot tell which repository it writes " + f"to. Failing closed. {unread[1]}" + _handoff(cmd) + ) # 5. Hand-rolled reply/resolve for a review thread, bypassing scripts/pr_review.py's one-call helper. dec, reason = _check_reply_resolve_helper(cmd, environ) @@ -8424,6 +8425,44 @@ def classify( ] +_NO_ORIGIN_CASES = [ + ("eval 'gh pr comment 5 -b x'", "deny", "no origin: a gh write an eval runs"), + ( + "gh api -X POST repositories/12345/issues -f title=t", + "deny", + "no origin: an api write naming its repository by id", + ), + ( + "gh api -X POST repos/example/repo/../../stranger/example/issues -f title=t", + "deny", + "no origin: a dot-segment api write", + ), + ( + "declare -x GH_REPO=stranger/example; gh pr comment 5 -b x", + "deny", + "no origin: a write taking a GH_REPO a declare -x sets", + ), + ( + "gh api -X POST repos/stranger/repo/issues", + "allow", + "no origin: a literal api target under another owner", + ), + ("gh pr comment 5 -b x", "allow", "no origin: a write naming no target"), + ( + "gh pr comment 5 --repo stranger/example -b x", + "allow", + "no origin: a literal repo flag under another owner", + ), + ( + 'gh pr comment 5 --repo "$R" -b x', + "allow", + "no origin: a run-time repo flag, which only the origin branch reads", + ), + ("gh api repositories/12345", "allow", "no origin: an api read by repository id"), + ("eval 'gh pr view 5'", "allow", "no origin: a gh read an eval runs"), +] + + # Rule-5 cases, covering the hand-rolled reply/resolve denial and its cross-owner grant escape. # Each carries the environment the grant is read from, matching the _SCOPE_CASES convention below. _REPLY_RESOLVE_CASES = [ @@ -15087,6 +15126,33 @@ def _push_repo_selftest(): return ok +def _no_origin_selftest(): + """Run `_NO_ORIGIN_CASES` with the origin read pinned to resolve nothing. + + That is the read outside any checkout. An unread target fails closed there, while a literal + or run-time target is allowed, since there is no owner to compare it against. The pin + replaces `_origin_owner_repo` rather than passing a directory, so no case depends on where + the self-test runs or on a git spawn. + """ + saved = globals()["_origin_owner_repo"] + globals()["_origin_owner_repo"] = lambda _cwd: None + ok = True + try: + for cmd, want, label in _NO_ORIGIN_CASES: + got, _ = classify( + cmd, + current_branch="feature/x", + rules_lookup=lambda br: set(), + environ={}, + primary_checkout_lookup=lambda d: False, + ) + ok = ok and got == want + print(f" {'ok ' if got == want else 'FAIL'} [{got:5}] want={want:5} {label}") + finally: + globals()["_origin_owner_repo"] = saved + return ok + + def _selftest(): # A deterministic offline run, pinning origin to ptr727/PlexCleaner, the incident repo, so the cross-origin case resolves without touching a real checkout. # The gh-write cases inject empty rules and a feature current-branch so no case reaches the live branch-rules query. @@ -15613,6 +15679,8 @@ def counting_lex(text, posix=True): ok = False if not _push_repo_selftest(): ok = False + if not _no_origin_selftest(): + ok = False if not _alias_push_selftest(origin): ok = False _PINNED_ALIASES.clear() From 848d629a1c2a960c4e98904895e978631eb8d798 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 20 Oct 2026 19:15:10 -0700 Subject: [PATCH 5/5] Fix and Pin the Docker Task Workflow Test Reader's Limits (#3296) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Closes #3280 The stdlib-only reader in `tests/test_docker_task_workflow.py` now closes the limits that need a small change and pins the rest. Fixed, each with a case that fails on the old reader: - A step whose first key is `run:` or `uses:`. - Docker global options before the subcommand. - A `with:` child indented deeper than ten spaces. - `--push=false` and `--push=0` no longer count as a push. - A `run` script ends at its first dedent, so a later `env:` value is not read as script. Pinned as known limits in `KnownLimitsCase`, each asserting the current behavior: heredoc, quoted separator, and trailing comment pushes, a `type=registry` cache source, folded and plain multi-line `run`, a multi-line flow `with`, a block-scalar login value, an inverted credential check, a check held in an echo, other missed push forms, and `with: push: false`. Two limits are listed in the header without a case: a bare `-` sequence item and a shallow `run` body. The header lists every remaining limit. No YAML dependency is added. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Sonnet 5.5 --- tests/test_docker_task_workflow.py | 182 +++++++++++++++++++++++++++-- 1 file changed, 175 insertions(+), 7 deletions(-) diff --git a/tests/test_docker_task_workflow.py b/tests/test_docker_task_workflow.py index 5a39e18af..c4d05aa25 100755 --- a/tests/test_docker_task_workflow.py +++ b/tests/test_docker_task_workflow.py @@ -5,6 +5,18 @@ that pushes, the caller's hook step included, and the hook step must receive the inputs it builds from. The job and step sets are read from the workflow, so a reordered, renamed, or added job is judged by the same rule rather than by a hard-coded index. + +The reader is an indentation reader on the standard library alone, not a YAML or shell parser. +Its remaining limits are listed here, and all but the last two have a `KnownLimitsCase` case below, so a change that alters one is noticed. +A push inside a heredoc body, after a quoted separator, or in a trailing shell comment is read as a push. +A `type=registry` cache source is read as a push. +A command folded across lines without a backslash is not joined. +A flow-style `with` mapping spanning lines is not read. +A block-scalar login value is not read as the secret. +An inverted credential check is accepted, and the secret assignments and `-z` tests are matched as text anywhere in the step. +A push by `docker compose push`, `imagetools create`, `--output type=image,push=true`, or a buildx-level option before `build` is missed. +`with: push: false` is read as a push, and only lowercase `--push=false` and `--push=0` are read as no push. +Two further limits have no case: a bare `-` sequence item ends the step list, and a `run` body less than two columns deeper than its key is not read. """ from __future__ import annotations @@ -68,6 +80,9 @@ def split_flow(body: str) -> list[str]: def normalize_item(item: list[str]) -> list[str]: """Re-indent one step to the block layout the readers expect and expand a one-line `with` mapping.""" + first = re.fullmatch(r"(\s*)- +((?!name:)[A-Za-z0-9_-]+:.*)", item[0]) + if first: + item = [f"{first.group(1)}-", f"{first.group(1)} {first.group(2)}", *item[1:]] shift = 6 - (len(item[0]) - len(item[0].lstrip())) lines = [ (" " * shift + line if shift >= 0 else line[min(-shift, len(line) - len(line.lstrip())) :]) @@ -130,15 +145,24 @@ def with_values(item: list[str]) -> dict[str, str]: """The keys and values directly under the step's `with:`, ignoring comments.""" values: dict[str, str] = {} in_with = False + child = "" for line in item: if re.fullmatch(r" with:\s*(#.*)?", line): in_with = True elif in_with: - pair = re.match(r" [\"']?([A-Za-z0-9_-]+)[\"']?:\s*(.*?)\s*$", line) + if not child and line.strip() and not line.lstrip().startswith("#"): + child = line[: len(line) - len(line.lstrip())] + if len(child) <= 8: + break + pair = ( + re.match(rf"{child}[\"']?([A-Za-z0-9_-]+)[\"']?:\s*(.*?)\s*$", line) + if child + else None + ) if pair: value = re.sub(r"\s+#.*$", "", pair.group(2)).strip("\"'") values[pair.group(1)] = re.sub(r"\$\{\{\s*(.*?)\s*\}\}", r"${{ \1 }}", value) - elif line.strip() and not line.startswith(" "): + elif line.strip() and not line.startswith(child): break return values @@ -180,9 +204,10 @@ def is_login(step: Step) -> bool: ) +DOCKER = r"\bdocker(?:\s+--?[A-Za-z][\w-]*(?:=\S+|\s+[^-\s]\S*)?)*?" PUSH_COMMAND = re.compile( - r"\bdocker\s+(?:(?:image|manifest|buildx)\s+)?push\b" - r"|\bdocker\s+(?:buildx\s+)?(?:build|bake)\b.*(?:--push\b|type=registry\b)" + rf"{DOCKER}\s+(?:(?:image|manifest|buildx)\s+)?push\b" + rf"|{DOCKER}\s+(?:buildx\s+)?(?:build|bake)\b.*(?:--push\b(?!=(?:false|0)\b)|type=registry\b)" ) @@ -193,9 +218,10 @@ def run_script(step: Step) -> str: match = re.fullmatch(r" run:\s*(.*)", line) if match: body = [match.group(1)] - body.extend( - rest for rest in lines[i + 1 :] if rest.startswith(" ") or not rest.strip() - ) + for rest in lines[i + 1 :]: + if rest.strip() and not rest.startswith(" "): + break + body.append(rest) return re.sub(r"\\\n\s*", " ", "\n".join(body)) return "" @@ -527,5 +553,147 @@ def test_login_value_comment_and_default_registry_are_clean(self) -> None: self.assertEqual([], violations(mutated)) +def before_login(step: str) -> str: + """The workflow with the step text written before the login step of the build-docker job.""" + return insert_before_login(DockerTaskWorkflowCase.workflow, "build-docker", step + "\n") + + +class ReaderFixCase(unittest.TestCase): + """Shapes the reader once missed or misjudged and now reads correctly.""" + + def test_first_key_run_or_uses_push_is_caught(self) -> None: + for step in ( + " - run: docker push example/image:tag\n name: Early push step\n", + " - uses: ./.github/actions/docker-build-example\n name: Early hook step\n", + ): + with self.subTest(step=step): + found = violations(before_login(step)) + self.assertIn("build-docker: no login before the first push step", found) + + def test_docker_global_options_before_push_are_caught(self) -> None: + for command in ( + "docker --config /tmp/example push example/image:tag", + "docker -H tcp://example:2375 image push example/image:tag", + "docker --debug buildx build --push .", + ): + with self.subTest(command=command): + step = f" - name: Early push step\n run: {command}\n" + found = violations(before_login(step)) + self.assertIn("build-docker: no login before the first push step", found) + + def test_with_child_indented_deeper_than_ten_is_read(self) -> None: + step = " - name: Early push step\n uses: example/push@v1\n with:\n push: true\n" + found = violations(before_login(step)) + self.assertIn("build-docker: no login before the first push step", found) + + def test_push_false_is_not_a_push(self) -> None: + for command in ("docker buildx build --push=false .", "docker buildx build --push=0 ."): + with self.subTest(command=command): + step = f" - name: Local build step\n run: {command}\n" + self.assertEqual([], violations(before_login(step))) + + def test_env_value_after_run_is_not_part_of_the_script(self) -> None: + step = ( + " - name: Quiet step\n run: true\n" + " env:\n NOTE: docker push later\n" + ) + self.assertEqual([], violations(before_login(step))) + + +class KnownLimitsCase(unittest.TestCase): + """Each case asserts the reader's CURRENT behavior for a shape it does not judge correctly.""" + + def test_limit_heredoc_body_push_is_flagged(self) -> None: + step = " - name: Doc step\n run: |\n cat < None: + step = ' - name: Echo step\n run: echo "a; docker push example/image"\n' + self.assertIn( + "build-docker: no login before the first push step", violations(before_login(step)) + ) + + def test_limit_trailing_shell_comment_push_is_flagged(self) -> None: + step = " - name: Comment step\n run: true # docker push example/image\n" + self.assertIn( + "build-docker: no login before the first push step", violations(before_login(step)) + ) + + def test_limit_registry_cache_source_is_flagged(self) -> None: + step = " - name: Cache step\n run: docker buildx build --cache-from type=registry,ref=example/cache .\n" + self.assertIn( + "build-docker: no login before the first push step", violations(before_login(step)) + ) + + def test_limit_folded_run_push_is_missed(self) -> None: + step = " - name: Folded step\n run: >\n docker\n push example/image\n" + self.assertEqual([], violations(before_login(step))) + + def test_limit_plain_multiline_run_push_is_missed(self) -> None: + step = " - name: Plain step\n run: docker\n push example/image\n" + self.assertEqual([], violations(before_login(step))) + + def test_limit_multiline_flow_with_push_is_missed(self) -> None: + step = " - name: Flow step\n uses: example/push@v1\n with: {\n push: true}\n" + self.assertEqual([], violations(before_login(step))) + + def test_limit_other_push_forms_are_missed(self) -> None: + for command in ( + "docker compose push", + "docker buildx imagetools create -t example/image:tag example/image:src", + "docker buildx build --output type=image,name=x,push=true .", + "docker buildx --builder b build --push .", + ): + with self.subTest(command=command): + step = f" - name: Early step\n run: {command}\n" + self.assertEqual([], violations(before_login(step))) + + def test_limit_uppercase_push_false_is_read_as_a_push(self) -> None: + step = " - name: Local step\n run: docker buildx build --push=FALSE .\n" + self.assertIn( + "build-docker: no login before the first push step", violations(before_login(step)) + ) + + def test_limit_push_false_input_is_read_as_a_push(self) -> None: + step = " - name: Local step\n uses: example/build@v1\n with:\n push: false\n" + self.assertIn( + "build-docker: no login before the first push step", violations(before_login(step)) + ) + + def test_limit_secret_assignment_held_in_another_value_is_accepted(self) -> None: + workflow = DockerTaskWorkflowCase.workflow + mutated = workflow + for name in CREDENTIAL_NAMES: + line = f" {name}: ${{{{ secrets.{name} }}}}" + mutated = mutated.replace(line, f' NOTE_{name}: "{line.strip()}"') + self.assertNotEqual(workflow, mutated) + self.assertEqual([], violations(mutated)) + + def test_limit_block_scalar_login_value_is_not_read(self) -> None: + password = " password: ${{ secrets.DOCKER_HUB_ACCESS_TOKEN }}" + workflow = DockerTaskWorkflowCase.workflow + mutated = workflow.replace( + password, " password: >-\n ${{ secrets.DOCKER_HUB_ACCESS_TOKEN }}" + ) + self.assertNotEqual(workflow, mutated) + self.assertIn("build-docker: no login before the first push step", violations(mutated)) + + def test_limit_inverted_credential_check_is_accepted(self) -> None: + joined = '-z "$DOCKER_HUB_USERNAME" || -z "$DOCKER_HUB_ACCESS_TOKEN"' + workflow = DockerTaskWorkflowCase.workflow + mutated = workflow.replace(f"[[ {joined} ]]", f"! [[ {joined} ]]") + self.assertNotEqual(workflow, mutated) + self.assertEqual([], violations(mutated)) + + def test_limit_check_tests_inside_an_echo_are_accepted(self) -> None: + joined = '-z "$DOCKER_HUB_USERNAME" || -z "$DOCKER_HUB_ACCESS_TOKEN"' + workflow = DockerTaskWorkflowCase.workflow + mutated = workflow.replace(f"if [[ {joined} ]]; then", f"echo '{joined}'; if false; then") + self.assertNotEqual(workflow, mutated) + self.assertEqual([], violations(mutated)) + + if __name__ == "__main__": unittest.main()