Skip to content

refactor(telemetry): replace the write-access check with a notice - #1115

Merged
ParidelPooya merged 5 commits into
mainfrom
feat/read-only-responsibility-notice
Oct 28, 2026
Merged

ParidelPooya merged 5 commits into
mainfrom
feat/read-only-responsibility-notice

Conversation

@ParidelPooya

@ParidelPooya ParidelPooya commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up to #1110 (issue #1107).

Problem

After the round-3 redesign in #1110, the write-access check no longer changed what the customer sees. Every result, can-write or unverified, led to the same warning and the same consent prompt. The check only changed the message text. It was also the most code in #1110, its Redshift queries were never verified on Redshift, and it produced every review round: definer functions, the Redshift LIKE escape, and sequences.

Cause

No catalog query can show that a SELECT cannot change data. So the check could never remove the prompt, and the extra detail did not justify its cost (CONTRIBUTING rules 1 and 2).

Options considered (rule 3)

  1. Keep the check as advisory text in the prompt. Rejected. It keeps every catalog query and its review surface, and it changes no outcome.
  2. Drop the MCP prompt and put a notice in the first tool result. Rejected. The agent can summarise a tool result away, so the user might never see the notice.
  3. Chosen: a fixed one-line notice, confirmed once per session in MCP, and a static line in Settings. The S3 result-location check stays, because it finds a misconfiguration that corrupts data whatever the credentials.

Change

  • core: delete checkWriteAccess, its catalog queries, WriteAccessResult, and the writeAccess field of DestinationTestReport. The S3 result-location check stays, moved unchanged to athenaResultLocation.ts. Test connection still skips its Athena SELECT 1 when results would land inside the table's data, or the location cannot be determined.
  • MCP: the gate asks once per process to confirm one line: "Queries are written by the agent and run with this server's credentials. It is your responsibility to give this server read-only credentials." For S3 + Athena, the gate also runs the location check. When results land inside the table's data, the finding is added to the prompt and to a blocked result. When the location cannot be determined (for example GetWorkGroup is denied), the prompt says the check did not complete and gives the reason. Elicitation, DURABLE_TELEMETRY_ACCEPT_WRITE_ACCESS, shared prompts and the cancellation check are unchanged. The checkbox field is confirmed (was acceptRisk).
  • VS Code and desktop: Settings shows a static warning under Test connection: "Queries are written by a model and run with these credentials. It is your responsibility to use read-only credentials." The wording matches the MCP line, adapted to the host. WriteAccessWarning and the renderer test runner it needed are removed, together with its CI step.
  • READMEs: describe the notice instead of the check. The MCP README explains the S3 note and why athena:GetWorkGroup is needed. The S3 + Athena IAM row keeps it.
  • Release: the MCP package is 1.1.0, with release-notes/telemetry-mcp-1.1.0.md and an entry under Unreleased in CHANGELOG.md.

Breaking change to the published 1.0.0 output (rules 1.3 and 7)

@aws/durable-execution-sdk-js-telemetry-mcp@1.0.0 includes #1110. Its test_destination output and blocked data-tool results carried a writeAccess field, which this PR removes. The blocked error and howToAccept text also changes. Agents read these results as text, so the version is a minor bump (1.1.0), and the release note states the change.

Decisions (rule 7)

  • MCP keeps the consent prompt rather than a notice in a tool result (option 2 above).
  • The S3 location check is kept in Test connection and in the MCP prompt. In the prompt it adds the finding when results land inside the data, and the reason when the location cannot be determined, so the user knows the check did not complete before confirming (raised in review). A location outside the data adds nothing.
  • The env variable keeps its name, DURABLE_TELEMETRY_ACCEPT_WRITE_ACCESS, so existing 1.0.0 configs keep working.
  • The Settings warning shows for every destination, including SQS and CloudWatch Logs. One unconditional line is simpler than per-destination rules.
  • checkAthenaResultLocation stays exported from core, because the MCP package uses it.

Edited test expectations (rule 5.4)

  • writeAccessGate.test.ts: the prompt and blocked text is now CONSENT_MESSAGE, "The client cannot show a confirmation prompt." and "The user did not confirm.", replacing the can-write / unverified text. The cases that depended on a check result are removed.
  • registerTools.test.ts: "blocked until the user confirms" replaces the two per-status blocked cases. test_destination no longer asserts a writeAccess field.
  • Both files: the elicitation reply uses confirmed instead of acceptRisk.

Testing

  • New: the gate adds a configuration warning to the prompt and to a blocked result. For S3, registerTools.test.ts checks four things: the finding appears when results land inside the data, the reason appears when the location is undetermined, nothing is added for outside, and other destinations never run the check.
  • Kept: every data tool is blocked until the user confirms, plus cancellation. The location cases are in athenaResultLocation.test.ts, and the probe-skip cases in destinationTest.test.ts.
  • Branch check (rule 4): a script removes or inverts each of 31 branches one at a time. Each mutation makes at least one test fail.
  • npm run typecheck:hosts passes. The webview type-checks and builds.
  • npm test passes in telemetry-core (476), telemetry-mcp (487), telemetry-vscode (34), telemetry-desktop (48).

…otice

Since the last redesign the write-access check never changed what the
customer saw: every result led to the same warning and the same consent
prompt. It was also the source of every review round (definer functions,
the Redshift LIKE escape, sequences). So remove it.

- core: delete checkWriteAccess, its catalog queries, WriteAccessResult,
  and the writeAccess field of DestinationTestReport. Keep the S3 result
  location check, now athenaResultLocation.ts: Test connection still
  skips its Athena SELECT 1 when results would land inside the table's
  data, because read-only credentials do not prevent that.
- mcp: the gate asks once per process to confirm one line ("It is your
  responsibility to give this server read-only credentials."), with no
  check. Elicitation, the env variable, shared prompts and the
  cancellation check are unchanged.
- VS Code and desktop: Settings shows the same line as a static warning.
  Remove WriteAccessWarning and the renderer test runner it needed.
- READMEs: describe the notice instead of the check.
@github-actions

This comment has been minimized.

….1.0

Review of the notice found that the MCP prompt no longer warned when
Athena writes query results inside the table's data, where every query
adds rows whatever the credentials. #1110's check had covered it.

- mcp: the gate takes an optional configurationWarning. For s3 it runs
  checkAthenaResultLocation and, when results land inside the data,
  appends the finding to the prompt and to a blocked result.
- mcp: the prompt's checkbox field is `confirmed` (was `acceptRisk`),
  and the gate docs say "confirm" instead of "accept the risk".
- vscode/desktop: Settings uses the same wording as the MCP line.
- mcp README: the S3 note, and why athena:GetWorkGroup is needed.
- Bump the MCP package to 1.1.0. Release notes and a changelog entry
  record that test_destination and blocked results no longer carry
  writeAccess.
@ParidelPooya
ParidelPooya deployed to ai-pr-review-runtime October 28, 2026 05:39 — with GitHub Actions Active
@github-actions

This comment has been minimized.

Every caller passes it, so the optional call had a branch no test could reach (rule 4).
@ParidelPooya
ParidelPooya deployed to ai-pr-review-runtime October 28, 2026 05:57 — with GitHub Actions Active
@github-actions

This comment has been minimized.

…ipped

Reword comments that still named the removed write check. Document that the MCP prompt says nothing about the Athena result location when it cannot be read, or when DURABLE_TELEMETRY_ACCEPT_WRITE_ACCESS skips the prompt; test_destination reports it in both cases. Pin one configuration check per shared prompt in the gate test.
@ParidelPooya
ParidelPooya deployed to ai-pr-review-runtime October 28, 2026 06:13 — with GitHub Actions Active
@github-actions

This comment has been minimized.

When checkAthenaResultLocation is undetermined (for example GetWorkGroup is denied), the MCP consent prompt now says the check did not complete and gives the reason, so the user knows before confirming. A test covers it.
@ParidelPooya
ParidelPooya deployed to ai-pr-review-runtime October 28, 2026 20:37 — with GitHub Actions Active
@github-actions

Copy link
Copy Markdown

Codex AI review

Two approach findings:

  • P1: Breaking output in a minor release — packages/aws-durable-execution-sdk-js-telemetry-mcp/package.json:3.
  • P2: S3 fallback contradicts the recorded decision — packages/aws-durable-execution-sdk-js-telemetry-mcp/src/registerTools.ts:84.

Static review only; runtime and renderer tests were not executed.

Suppressed repeats

  • arf_v1_nasztqddri7alg4lt6luitdkif — deferred (maintainer verdict)

Reviewed commit d4ce9376bf5cacd3cd43dcb0ca46773d6293df5b. Workflow run

@yaythomas yaythomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@ParidelPooya
ParidelPooya merged commit 6d024cb into main Oct 28, 2026
52 checks passed
@ParidelPooya
ParidelPooya deleted the feat/read-only-responsibility-notice branch October 28, 2026 22:57
ParidelPooya added a commit that referenced this pull request Oct 28, 2026
#1115 removed the renderer test runner and its CI step along with its only
test. The execution viewer adds renderer tests, so this restores both.
ParidelPooya added a commit that referenced this pull request Oct 29, 2026
…ts env var (#1121)

This change was split out of #1119, as the review there asked. It
replaces the "Don't ask again" option from #1119 with simpler changes to
the credential prompt from #1115.

## Problem

1. A user who has already confirmed the credential prompt is asked again
in every new server process. The prompt does not say how to stop that.
2. In Claude Code, a user who reads the prompt and presses Submit is
refused.
3. In Claude Code, the end of the prompt is cut off.

## Cause

1. The only lasting route is an environment variable,
`DURABLE_TELEMETRY_ACCEPT_WRITE_ACCESS`. The prompt does not mention it.
Only a blocked result names it. The name also reads as accepting write
access, while the user confirms the opposite.
2. The form has one checkbox that defaults to unchecked. I traced the
MCP traffic of Claude Code 2.1.312. A plain Submit sent
`{"action":"accept","content":{"confirmed":false}}`. The server counts
an unchecked box as a decline.
3. Claude Code 2.1.312 showed about the first 169 characters of the
prompt. The prompt was 141 characters before this change, and adding the
variable sentence made it 269.

## Options considered

1. **Chosen: name the variable in the prompt, remove the checkbox, and
shorten the prompt.** Each part removes one cause, and the change
removes more logic than it adds.
2. **A "Don't ask again" checkbox that saves the answer to a file.**
Rejected. It adds a store module, a second confirmation route and their
tests. Its only gain over the env var is one config edit per
configuration (rule 1.3). It also adds a second checkbox to a form that
already failed with one.
3. **Keep the checkbox and explain the refusal.** Rejected. The user
still has to answer a second prompt, and the checkbox adds nothing that
the Accept button does not already say.

## Change

- **Renamed:** `DURABLE_TELEMETRY_ACCEPT_WRITE_ACCESS` is now
`DURABLE_TELEMETRY_CONFIRMED_READONLY`. The constant
`ACCEPT_WRITE_ACCESS_ENV_VAR` is now `CONFIRMED_READONLY_ENV_VAR`.
- **Prompt text:** "I confirm that this server's credentials are
read-only. To stop asking, set
DURABLE_TELEMETRY_CONFIRMED_READONLY=true." That is 118 characters. On
S3 + Athena, the configuration warning still goes between the two
sentences.
- **Form:** no fields (`{ type: "object", properties: {} }`). Accept
confirms. Decline or cancel refuses.
- **Blocked result:** `error` states only the reason, for example
"Refused to run queries. The user did not confirm that this server's
credentials are read-only." The first-person prompt line would read
wrong there. A configuration warning is still added. `howToAccept` names
the new variable.
- **README and 1.1.0 release notes:** they describe the new prompt and
the rename.

## Decisions (rule 7)

- **The old variable name is not read, and there is no alias.** The
environment variable is not part of the MCP server's compatibility
contract. The 1.1.0 release notes tell users who set the old name to
rename it.
- **The hint is added in `askUserToAccept`, not to `CONSENT_MESSAGE`.**
A blocked result already names the variable in `howToAccept`.
- **A test keeps the prompt at 160 characters or fewer.** The limit
leaves a margin below the 169 characters that Claude Code showed.
- **A submitted form confirms whatever its content.** The form asks for
no fields. So a client that still sends `confirmed: false` does not
refuse the user.
- **The S3 + Athena warning stays in the prompt.** It can be longer than
the visible limit, so its end and the hint can be cut in Claude Code.
Moving it out of the prompt changes #1115's design, so it is left for a
separate change.
- **The environment variable still skips the S3 + Athena location
check.** That is the behavior on `main`.

## Edited tests (rule 5.4)

In `writeAccessGate.test.ts`:

- `asks the user to confirm the read-only responsibility`: now asserts
the new `CONSENT_MESSAGE` exactly.
- `blocks with the message when the user declines`, renamed `blocks with
the reason when the user declines`: now asserts the new `error` text
exactly.
- The two "cannot show a prompt" tests: the reason text changed.
- `accepts only when the user checks the box`, renamed `asks with a form
that has no fields, and accepts on submit`: now asserts the empty
schema.
- `treats a submitted form with the box unchecked as a decline`, renamed
`accepts a submitted form whatever its content`: the reply that Claude
Code sent now confirms.
- `shows the prompt and accepts when the user checks the box`, against
the real MCP SDK, renamed `shows the prompt and accepts when the user
submits it`: asserts the empty schema and the hint.
- `rejects a reply that does not match the form schema`: removed. A form
with no fields has nothing to validate.

In `registerTools.test.ts`, `does not reach its query path until the
user confirms` now matches the new refusal text.

## Testing

- MCP: 487 tests pass. Typecheck and biome pass.
- With the old gate code, the three form tests fail. With the hint
removed, the two hint tests fail.
- Claude Code 2.1.312, with the built server and MCP traffic logged:
- before this change, a plain Submit sent `confirmed: false`, and the
server refused;
- with the empty form, Submit sent `{"action":"accept","content":{}}`,
and `list_executions` returned executions;
- with `DURABLE_TELEMETRY_CONFIRMED_READONLY=true`, no prompt was shown,
and executions were listed.

This branch was successfully deployed

1 active deployment
ai-pr-review-runtime — d4ce9376 Deployed Oct 28, 2026 by ParidelPooya via ai-pr-review / Codex review / Generate Codex review #2306
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants