Repository navigation
feat: [SDK-5480] make each checkpoint one command and pre-approve the helpers in Claude Code - #66
Conversation
042e4ae to
6ca783a
Compare
kalley
left a comment
kalley
left a comment
There was a problem hiding this comment.
Moving the state writes into checkpoint.sh is a clean way to make each checkpoint one command. Two non-blocking notes inline.
| bash <plugin>/scripts/checkpoint.sh credentials.detected fail unknown probe_unavailable # the check cannot run (item 7) | ||
| ``` | ||
|
|
||
| On a direct run, append `--app-id <APP_ID>` to the line you pick ("Checkpoint consent" above): the script records the App ID and flushes the events held before it. |
There was a problem hiding this comment.
This also applies to the invalid_app_id line above. The script would then write the App ID that the probe just rejected, and send under it. If that ID isn't UUID-shaped, the script refuses the call and the fail row is lost. Should the fail line skip --app-id?
| ``` | ||
|
|
||
| After a "send" answer, once `.onesignal/app_id` is written, run `bash <plugin>/scripts/checkpoint.sh flush` once: this funnel run may hold events that buffered before the answer existed, and the script keeps them for exactly this recovery (telemetry contract, "Refusal and failure behaviour"). | ||
| The script writes the UUID to `.onesignal/app_id` at the repo root, sends that event, and then flushes the events held from before the answer existed — the script keeps them for exactly this recovery (telemetry contract, "Refusal and failure behaviour"). Every milestone call accepts `--app-id`, so pass it on whichever checkpoint comes first. Do not write the file by hand, and do not run a separate `flush` for it: one command keeps the host's approval prompts to one. |
There was a problem hiding this comment.
checkpoint.sh:270 still says "Write it to $STATE_DIR/app_id first" when flush has no App ID, and :277 says the same for a bad one. Could those point at --app-id too, so the script doesn't tell the agent to do what this line forbids?
abdulraqeeb33
left a comment
abdulraqeeb33
left a comment
There was a problem hiding this comment.
The script changes look solid. One question inline about how far the onesignal_api.py grant reaches.
| argument-hint: "[platform=ios|android|web|email|sms] [app=<APP_ID>]" | ||
| allowed-tools: | ||
| - Bash(bash ${CLAUDE_PLUGIN_ROOT}/scripts/checkpoint.sh *) | ||
| - Bash(python3 ${CLAUDE_PLUGIN_ROOT}/scripts/onesignal_api.py *) |
There was a problem hiding this comment.
onesignal_api.py * also matches provision, the write-once credential upload to the live app. That is the one helper call that mutates customer state and can't be undone through the API. With this rule, the consent question becomes the only gate before it runs. AGENT.md and the README describe the grant as the helper scripts, so this is easy to miss. Consider listing the read subcommands instead (app *, web-probe *, android-params *, subscribers *, notification-stats *), so provision keeps the host prompt. That costs one prompt per upload, right after the user has already said yes. The same rule is at skills/setup/SKILL.md:10.
… helpers in Claude Code
Claude Code prompts for every Bash command that no allow rule matches,
and a compound command never matches: the Step 1 and Step 2 checkpoint
blocks started with a variable assignment and an `&&` chain, so each
one prompted even with a rule for `checkpoint.sh`.
- checkpoint.sh takes `--platform <token>` and `--app-id <uuid>`. The
script writes `.onesignal/platform` and `.onesignal/app_id` at the
repo root, validates both values, and flushes the buffer after the
`--app-id` event sends. A skill step is now one `checkpoint.sh` call.
- The 3 skills list their helper scripts in `allowed-tools`, with
`${CLAUDE_PLUGIN_ROOT}` so the rule matches the command the body runs.
The grant covers the invoking turn; the README says what still asks.
- The bundle gate and the CI portability grep exempt the `allowed-tools`
frontmatter lines and nothing else; a new self-test case pins that.
- The telemetry contract and AGENT.md document the options and the
one-command rule.
… --app-id line in credentials and verify - An unknown `--option` used to fall through to the positionals, become a failure class, be rewritten to `unknown`, and send a wrong row while the App ID it carried was lost. The parser now refuses it with exit 0. - `flush` refuses `--platform` and `--app-id` instead of dropping them. - The `--app-id` block only runs the auto-flush; the case arm above already reports a refused send, so the second message is gone. - The credentials and verify command blocks say to append `--app-id` on a direct run, and the verify text names the section the checkpoint lives in.
…call; compute the self-test line - The --app-id auto-flush child repeated the parent's consent_env_only warning. The child now receives ONESIGNAL_SKILL_CONSENT_WARNED=1 and stays quiet; a flush run by hand still warns once. - The frontmatter self-test case finds the argument-hint line instead of pinning line 4, so a reordered frontmatter does not fail it for the wrong reason.
6ca783a to
9a181fa
Compare
One Line Summary
Consolidates multiple checkpoint commands (via
checkpoint.sh) into one call and adds a pre-approvals for helper scripts in each skill. The end result is that Claude Code users will experience far fewer permission prompts to run Bash commands.Motivation
In default mode, Claude Code asks before every Bash command, e.g.
checkpoints.sh. The Step 1 and Step 2 checkpoint blocks insetup, and the App ID block incredentialsandverify, were compound commands that weren't caught under the umbrella permissions ruleset. This means that they prompted users for permission on every run even when the user had allowedcheckpoint.sh. For users, this meant needing to babysit a run and constantly approve permissions for the plugin to take actions.The result of this change should be a 50-75% reduction in number of permission request prompts that the user will have to field.