feat: show what a chat lifecycle hook changed - #27655
Conversation
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
|
@codex review |
![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: 0017be0fed
ℹ️ 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".
0017be0 to
9e7825f
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e7825fd3d
ℹ️ 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".
ibetitsmike
changed the title
ibetitsmike
changed the title
ibetitsmike
changed the title
9e7825f to
83e6f0c
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83e6f0c73d
ℹ️ 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".
83e6f0c to
278af8e
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 278af8ec04
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 278af8ec04
ℹ️ 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".
278af8e to
3904bbb
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3904bbb5f2
ℹ️ 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".
3904bbb to
92de546
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92de546247
ℹ️ 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".
92de546 to
5fb46b8
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fb46b8c09
ℹ️ 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".
5fb46b8 to
28381f5
Compare
Tool headers now read `hookRewritten` from the context `ToolCall.Root` already creates, instead of a separate exported `PolicyProvider` that every caller had to remember to wrap. Renderers forward the flag to their root the same way they already forward `status` and `isError`. The two rows that render a bare `ToolCall.PolicyBadge` (the execute auth-required card and the answered ask_user_question row) now sit inside a root, so a badge rendered outside one throws instead of silently rendering nothing.
The registry-wide story counted rendered cases, so the extra subagent and unknown-tool fixtures could offset a registered renderer that produced nothing and the count assertion still passed. It now records which registry names actually rendered and requires all of them. Folds the negative auth-required badge assertion into the story it duplicated, and makes the internal renderer prop required now that `Tool` normalizes the public one.
c0740e0 to
d51a378
Compare
|
Correction to my previous comment, and a rebase note. Main had drifted into a conflict with this branch, which meant GitHub could not The rebase invalidated part of what I told you. Two of the files I described were
So one of the two "badge outside a root" sites no longer exists. I described Corrected numbers for the threading cost, since I quoted the pre-rebase ones:
Everything else in my previous comment stands: the flag lives on the context Post-rebase validation:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d51a378ea2
ℹ️ 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".
FE10 forbids `querySelector` in tests. The registry-wide badge story scanned a private `data-policy-case` attribute and cast each node, so a markup change could silently stop the assertions from finding anything. It now walks the fixture table and looks each case up with `getByTestId`, which is the sanctioned query for a wrapper that has no semantic role.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df3bf63067
ℹ️ 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".
…alls Hooks never see provider-executed tool calls, so a provider-executed call that reused an admitted call's ID inherited hook_rewritten and the API attributed it to policy. Duplicate-ID rejection deliberately skips provider-executed calls, so the collision reaches conversion.
Threading `hookRewritten` from `Tool` into each renderer and on into every `ToolCall.Root` touched 21 files to express one cross-cutting display flag. `ToolCall.PolicyProvider` returns, and the provider now renders the badge itself instead of relying on the shared header. Emitting the badge at the provider closes the gap the prop threading was chasing: a renderer branch that returns early without a `ToolCall.Header`, such as the auth-required execute card or an empty `ask_user_question` payload, keeps the attribution without participating at all. Renderers no longer see the flag, and 15 of them leave the diff entirely. A rewritten call is wrapped in a group labelled by its badge, so a nested row such as one file inside a merged read carries its own attribution. `ReadFileTool` names its header row, which had been relying on the badge sitting inside the button to describe the row.
The coverage loop used a text query, which matches a badge the card's own layout hides. Verified by hiding the badge with display:none, which the previous assertion accepted.
2b18c61 to
c8259c6
Compare
The fallback that renders merged tools without a parsed block omitted hookRewritten, so a rewritten call reaching that path lost the badge.
|
Reverted the prop-piping refactor from the previous round. Mike pushed back: threading That closes the defect class you found three times ( Also in this round:
One pre-existing issue I am deliberately not fixing here: @codex review
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa411153eb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The previous commit gave ReadFileTool an explicit ariaLabel so a grouped row could be told apart by name. An explicit aria-label replaces the name computed from descendants, dropping the failure text ToolCall.Status contributes, so a failed read announced only the filename. The grouped story reads attribution from the policy group instead, and ReadFileFailed guards the failure name.
|
Fixed the read-file accessible name regression from the last round ( That one was mine: I had added an explicit @codex review
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61b94b96b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The story seeded workspaceByIdKey with an untyped partial, so a change to the Workspace or WorkspaceBuild shape could not surface here. It now composes MockWorkspace and MockWorkspaceBuild under an explicit TypesGen.Workspace annotation, which rejects the previous partial.
|
Fixed the fixture finding in The policy story now composes That change also surfaced a latent inconsistency in my own fixture: @codex review
|
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Stack Context
Follow-up fixes from live UAT of the merged chat lifecycle hooks stack (#27430). Two PRs:
mike/chat-hooks-uat/dispatch-capacity]: reserve dispatch capacity so an admission burst can't fail running turns.Why?
UAT found three ways the transcript misrepresented what a lifecycle hook did. All three are user-visible and share the same surface (
chathooks/effects.go,codersdk.ChatMessagePart, the conversation timeline), so they're reviewed together.A prompt
input_overridesilently discarded attachments.ComposeUserPromptContentreplaced the entire submitted part list with one text part, droppingfileandfile-referenceparts along with theirchat_file_links. The user saw their attachments vanish with no explanation. The override now replaces submitted text parts only and preserves non-text parts in order. A consumer that wants to block attachments usesdeny, which is the documented mechanism for refusing a submission.Every user-visible
systemrow was labelled "Lifecycle hook". The timeline keyed the notice offrole === "system". That was correct only by accident, because the hookuser_messagewas the sole client-visible system row. The backend now emits the notice as a typedhook-noticepart and the timeline renders on that, so a future system row can't be mislabelled as a policy notice.Nothing marked a tool call the hook had rewritten. A consumer could replace tool input via
input_overrideand the transcript showed the rewritten input as if the model had produced it.ChatMessagePartgainshook_rewritten, set frompreflight.Overrideson the same path that already carriesToolCallCreatedAt, and the tool row renders a "Modified by policy" badge.ToolCall.PolicyProviderrenders the badge itself, at four wrap sites: theTooldispatch wrapper, theReadFilesToolaggregate and its per-file rows, andReadFileTimelineBlock(grouped and singleread_filerows bypassTool). Renderer props do not include the flag; descendants consume it through the provider context.The badge is emitted by the provider rather than by the shared header because several renderer branches return early without one, including the auth-required
executecard, a completedask_user_question, and an empty question payload. Those branches would drop the attribution with no type or runtime error, and the gap is not greppable: every renderer file contains a header somewhere, only individual branches do not. Emitting at the provider removes the possibility instead of enumerating the cases.A rewritten call is wrapped in a group labelled by its badge, so one rewritten file inside a merged read is attributed on its own rather than inheriting the group's badge.
HeaderButtonstill appends the policy wording to an explicitariaLabel, since an explicitaria-labelreplaces the name computed from descendants.Provider-executed calls are excluded from attribution. Hooks never see them, and duplicate tool-call ID rejection deliberately skips them, so a reused ID would otherwise mark a provider-executed call as policy-rewritten.
Testing
Go:
coderd/x/chatd/...,coderd/x/agenthooks/...,codersdk/..., andcoderd -run 'Hook|Chat'. Frontend:tscplus everyAgentsPagestory; the only failures areMCP Tool CompletedandScroll To Bottom Button Works With Inverse Scroll, both of which fail on trunk.A registry-wide story asserts every registered renderer shows the badge, verified against three inverted toggles: removing the badge, hiding it with
display:none, and skipping the provider for one renderer (which names that renderer). Storybook also covers the rewritten subagent spawn, a completed empty question payload, a non-hook system message, and a failedread_fileguarding the accessible name.