Skip to content

fix: address #535 and #544 review - #548

Merged
moonlight16 merged 2 commits into
rossoctl:mainfrom
moonlight16:fix/delegate-skill-review
Oct 18, 2026
Merged

moonlight16 merged 2 commits into
rossoctl:mainfrom
moonlight16:fix/delegate-skill-review

Conversation

@moonlight16

Copy link
Copy Markdown
Member

Follow-up to #535 (moca-delegate-task rebuilt on mocactl) and #544 (Kind demo for the skill), from Paolo's review.

  • Permissions: SKILL.md and the README now say plainly that the allowlist removes Claude Code's prompt before ctx pack and workloads create, the two steps that upload local files, and offer settings.restricted.example.json, which keeps those two on prompt.
  • Local copy: the skill packs into a mktemp -d directory outside the working tree and deletes it at cleanup.
  • Naming: the flow doc and diagram name POST /v1/runs.
  • Demo isolation: setup.sh and run.sh pin SH_CONTROL_PLANE_URL and SH_HARNESS_URL to the demo, so a real deployment's exported URLs never receive the demo token or upload.
  • Demo safety: --delete removes only a state directory carrying the demo's marker, and --skip-build fails fast if the Context Service image isn't on the cluster.
  • deploy/k8s/setup.sh honours KIND_CLUSTER (default moca), with a comment that the demo depends on it.

🤖 Generated with Claude Code

- Say plainly that the skill's allowlist removes Claude Code's prompt before uploads, and offer a restricted set that keeps ctx pack and workloads create on prompt.
- Pack into a temporary directory outside the working tree and delete it at cleanup.
- Name POST /v1/runs in the flow doc and diagram.
- Pin the demo's SH_CONTROL_PLANE_URL and SH_HARNESS_URL so a real deployment's exported URLs never receive the demo token or upload.
- --delete removes only a marked demo state directory; --skip-build fails fast when the Context Service image is missing.
- deploy/k8s/setup.sh honours KIND_CLUSTER (default moca).

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Jeremy Cohn <Jeremy.Cohn@ibm.com>

@ingpaolodettori-dev ingpaolodettori-dev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Follow-up to our #535/#544 reviews. I checked all six items against the code at 5474f2f and they're addressed:

  • Permissions: SKILL.md and the README now say plainly that the full allow-list removes the prompt before ctx pack/workloads create. The new settings.restricted.example.json keeps both of those on prompt (it allows only spawn/subagents/get/workloads get/delete).
  • Local copy: the packed file goes in a mktemp -d dir and is removed at cleanup.
  • POST /v1/runs is now used in the flow doc and the SVG, matching leaf-turn.ts.
  • run.sh and setup.sh pin SH_CONTROL_PLANE_URL/SH_HARNESS_URL to the demo. That's the right fix, because mocactl ranks env over config (config.ts:192-194).
  • --delete uses the marker guard, and --skip-build fails fast through crictl inspecti.
  • deploy/k8s/setup.sh now reads KIND_CLUSTER from env (default moca), and KIND_CONTEXT is derived from it, so the demo's post-source override still works.

One suggestion: the new mktemp -d/rm -r steps aren't in either allow-list, so "Nothing prompts" isn't accurate any more. One nit on the marker refusal message. CI is green, and the single commit is signed off.

Comment thread skills/moca-delegate-task/SKILL.md Outdated
`~/.claude/settings.json`:

- [`settings.example.json`](settings.example.json): every delegation command, including the upload.
Nothing prompts.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion — "Nothing prompts" is no longer true after this PR. Step 1 now runs mktemp -d and step 6 runs rm -r TMP_DIR, and neither settings.example.json nor settings.restricted.example.json allows them. So with either set, Claude Code still prompts (or falls back to the classifier, which is what #530 is about) on every delegation. The same goes for "Delegation needs only these commands" (L137). Suggest adding "Bash(mktemp -d)" to both example files and to the JSON block below. Leave rm -r off the allow-list, since a wildcard rm -r * rule is much broader than this skill needs. Then say here that the cleanup rm -r TMP_DIR is the one command that still asks.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done in bc2a769.

Comment thread skills/moca-delegate-task/demo/setup.sh Outdated
rm -rf "$STATE_DIR"
# Only a directory this script made: a mis-set MOCA_DEMO_STATE must never be removed.
if [ -e "$STATE_DIR" ]; then
[ -f "$STATE_DIR/$STATE_MARKER" ] || die "refusing to delete $STATE_DIR: it has no $STATE_MARKER marker"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit — anyone who ran the #544 demo before this change has a state dir with no marker, so their first --delete after upgrading deletes the cluster (L74) and then dies here. The refusal itself is right. The message could also say what to do, e.g. …marker; if it is the demo's, remove it yourself (rm -rf $STATE_DIR), so it doesn't read like the cluster delete failed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done in bc2a769.

rossoctl#548 review)

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Jeremy Cohn <Jeremy.Cohn@ibm.com>
@moonlight16
moonlight16 merged commit a24aa0e into rossoctl:main Oct 18, 2026
15 checks passed

@ingpaolodettori-dev ingpaolodettori-dev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Follow-up at bc2a769: both items from the 5474f2f review are addressed. Bash(mktemp -d) is now allowed in both example settings files and the SKILL.md block, the doc states that the cleanup rm -r TMP_DIR still asks, and the --delete refusal message now explains the cluster was deleted and how to remove a pre-marker state dir by hand. No new issues.

pdettori added a commit that referenced this pull request Oct 20, 2026
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants