fix: surface and recover skill-command permission failures (#232, #233) - #235
Merged
Conversation
These were added to the tree by accident during a debugging session. They are per-machine state (agent worktree pointers, personal permission overrides) and never belong in the repo. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ected The suite gated on `/health`, which is unauthenticated — so a reachable ZeroID with an expired or revoked credential passed the readiness check, then died in the first test. That left `owner` and `conductor` unset and cascaded `undefined is not an object` into every dependent test: six failures reported as assertion errors when the real cause was a 401. Probe a cheap authenticated read (`agents.list()`) as part of readiness and skip with a diagnostic naming the actual reason, which is what the file's own docstring already promised it did. Also fix `test:integration`, which globbed `src/tests/*.integration.test.ts` and matched nothing — the only integration test lives in `src/integration/`, so it was reachable by neither `test` nor `test:integration`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two failure modes let a turn disappear without a trace. 1. A pushed prompt did not trigger an assistant turn. `shouldQuery` was unset on the SDK user message, so the SDK appended it to the transcript WITHOUT querying and merged it into the next querying message. The primary prompt now always queries; a "later" mid-turn injection still merges into the running turn, which is what it is for. 2. When a turn genuinely did not run, we reported success. A blocked slash-command expansion makes the SDK return `subtype: "success", is_error: false, num_turns: 0, result: ""` — a turn that never happened, dressed as a success. Nothing surfaced to the user, and no retry or error path ever fired. Treat `num_turns === 0` as an error regardless of `subtype`, and attribute it to the `<local-command-stderr>` that caused it when the SDK reports one. That cause arrives on an earlier message than the result, so it is carried across calls in a caller-owned `TranslateState` — one bag per session, so two concurrent sessions can never share it. Keying on `num_turns` rather than string-matching the stderr is deliberate: it is a typed field with correct data, it catches any consumed-but-unrun prompt whatever the cause, and it degrades to a generic message rather than back to silence if the upstream string ever changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…233) A `SKILL.md` may contain `` !`…` `` substitutions that run at slash-command EXPANSION time, before the agent loop starts, to inject content into the prompt. That phase has no approval path in a headless session: `canUseTool` is not consulted pre-loop and there is no human at a terminal, so anything not pre-allowed is hard-denied. One denial aborts the WHOLE expansion, and the prompt is then consumed with nothing to run — silent and total, and reported as a success by the SDK (#232). `options.allowedTools` governs that expansion-time check, so this is fixable in code: derive one allow rule per substitution the installed skills declare and pass them in. No mutation of the user's repo, no asking anyone to hand-edit `.claude/settings.json` — which also sidesteps the trap that a rule in `settings.local.json` is silently ignored for this check. Rules are emitted VERBATIM, no wildcards. Measured behaviour: the checker splits compound commands and requires every part allowed, OR one exact rule covering the whole string. The exact rule is strictly least-privilege, so a skill gets the one command line it declared and nothing else — never a prefix grant like `Bash(sh:*)`, which would hand every session arbitrary shell. Two parsing guards, both found against the real skill set: - the match excludes newlines. Prose like "…fails!`code`" would otherwise swallow everything up to the next backtick anywhere later in the file, turning whole paragraphs into "commands". - the first token must look like an executable name. Prose documenting the `!` character itself parses as a substitution (`release-notes/SKILL.md` yielded `Bash(suffix,)`). Both err toward granting less. That is the safe direction: an under-grant fails loudly via the zero-turn backstop, whereas an over-grant silently pre-approves a command for in-loop use. Rules are recomputed per query, so editing a skill can never leave a stale grant behind. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The skip message I added claimed an expired or revoked credential. That was a guess and it was wrong. Probing directly: ZeroID at :8899 is healthy, and `POST /api/v1/agents/register` returns 422 — the route exists — yet the SDK's `agents.register()` returns 404, with or without a valid API key. Cause: in @highflame/sdk 0.3.18 `AgentsResource.basePath` is "/agents" while every sibling resource is "/api/v1/..." and the server serves /api/v1/agents/*. Every agents.* call 404s. 0.3.18 is the latest published version, so there is nothing to upgrade to. Report the probe's actual error instead of asserting a cause, so the skip stays truthful whatever the underlying reason turns out to be. Worth noting this is not test-only: src/daemon/agent-identity.ts calls agents.register in three places, so agent identity registration is broken against any ZeroID serving /api/v1/agents/*. Tracked separately. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
, L4) Auto-granting every command an installed skill declares makes "installing a skill" mean "its shell runs unprompted". That is already true in interactive Claude Code, where a human clicks approve — but codeoid is headless, so the human was silently cut out. Put them back in. Only commands with a persisted approval are granted. Verdicts live in a new `skill_command_grants` table keyed by workspace + the VERBATIM command, so editing a skill changes the key and the modified command is re-asked rather than inheriting the old verdict. Denials are retained so a "no" is not re-prompted every turn. Approval is DEMAND-DRIVEN: we raise it for the command a denial actually blocked on, parsed out of the `<local-command-stderr>` the SDK reported. Prompting for everything *declared* would fire one prompt per undecided command across the whole skills tree — 31 on this developer's machine — for a turn that needed exactly one of them. Asking only for what blocked keeps it to a single prompt the user can connect to the thing they just ran. Deferred rather than blocking: these commands run at expansion time, before the agent loop exists, so there is no in-turn moment to await a decision without making runTurn async across all nine backends. The cost is that first use of a new command fails once — but loudly and actionably now, via the zero-turn backstop (#232), and it works from the next message. Unattended sessions need no special case: `autonomous` mode already auto-approves at the canUseTool gate, so the verdict resolves and persists on its own, while `guarded`/`interactive` genuinely ask. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Restores additionalDirectories, which I removed two commits ago for lack of evidence. That judgment was wrong — the evidence existed, it was just masked. Running a skill's command and reading the files that command touches are two independent gates, and they fail in that order. While every command was still being denied at the permission gate, the sandbox never got a chance to fail, so its absence looked like proof it was not needed. Once skillCommandAllowRules got a command through, the next failure was immediate and unambiguous: cat in '~/.codeoid/packs/ai-factory/skills/templates/requirement-template.md' was blocked. For security, Claude Code may only concatenate files from the allowed working directories for this session: '/…/highflame-zeroid' A pack installs skills as symlinks, and a skill's substitutions read siblings in its own tree (templates/, partials/). The sandbox resolves the symlink and sees a path outside the workdir. Discovery via settingSources is not access. Grants each skill's resolved PARENT — the `…/skills` root holding templates/ and partials/ — never the repo or pack it is symlinked out of. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ted tool path (#233) The approval rode `canUseTool`, so `autonomous` sessions auto-approved it and no dialog was ever rendered — a command was granted and persisted without the user seeing anything. Pipeline phases run autonomous, so in practice the human was never actually in the loop. Session mode is the wrong axis for this gate. Mode expresses how much you trust the AGENT's judgment for a task; this asks whether you trust code an installed PACK runs unconditionally, fixed at install time and invisible in the tool stream. Those are orthogonal, and conflating them reproduced exactly the silent-shell class this work exists to close. Route it through `requestUserInput` instead, which the auto-approve path cannot swallow, and treat `cancelled` as "no answer, never consent" — dismissal persists nothing and we ask again, rather than recording a deny the user never chose. Asking unconditionally is safe here because the approval is deferred: the turn has already failed by the time we ask, so an unanswered dialog blocks nothing. Worst case the skill stays blocked, which is the right default. The known cost is that a session with no client attached accumulates unanswered dialogs and those skills stay blocked — safe, but worth knowing. Verdicts stay keyed by workspace (workdir + tenant) and the verbatim command, so a new session in the same workdir does not re-ask, while a relative command like `sh .aif/partials/ethos-include.sh` is re-asked in a different repo, where it genuinely means a different file. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ecute hole (#233) loadPack already enforces the trust model for command gates: a pack is pure data, declaring a shell command is not executing it, and an untrusted pack's command gates fail closed until an operator opts in. Skill linking bypassed that. install() linked EVERY pack's slash-skills into ~/.claude/skills regardless of trust — and a skill's `!`…`` frontmatter runs a host shell at slash-command expansion time. So an untrusted pack's declared shell became runnable with no opt-in, the exact thing the gate path forbids. Two call sites had the gap: - install() linked unconditionally → now links only when trusted. An untrusted pack still installs and indexes; it just contributes no runnable slash-skills. - trust() recompiled gates but never linked skills → toggling trust ON was a no-op for skills, leaving a just-trusted pack half-installed. It now links on the ON transition. The pre-existing tests encoded the bug — installing `trusted: false` and then asserting skills WERE linked. Corrected to trusted, plus two tests locking the fix: an untrusted install links nothing, and trusting an installed-untrusted pack links its skills without a re-install. Note this is per-registry linking gated by per-pack trust, which is sound only because linking never clobbers and untrusted contributes nothing. A registry hosting packs at mixed trust cannot express "trust this skill, not that one" — tracked separately as registry-level trust. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ine (#233) A blocked skill command reported the turn as a terminal error, and the approval was fire-and-forget. In a pipeline that was fatal: the phase runner treats any non-idle turn as a phase failure, so `/spec` died with "phase turn ended in error" the instant it blocked — while the approval dialog was still open. The user approved, the grant persisted, and nothing happened: the run was already halted, and even a manual re-send would only re-run the skill outside the dead pipeline. Three symptoms, one cause — a recoverable "waiting for a human" state was being reported as a terminal error. Model it on elicitation instead, which the phase runner already treats as "still working" (waiting_approval resolves no phase waiter): - A zero-turn caused by a blocked skill command now emits `approval_pending` rather than a terminal turn_done. The session goes waiting_approval and posts a visible cue; the turn does NOT end and the pipeline phase stays alive. - On approval the provider retries the SAME prompt IN PLACE: the query loop rebuilds (skill grants are now part of its reuse guard, so the just-granted command reaches allowedTools) and the prompt is re-pushed into the same turn queue. From the session's view one turn simply resumes — no second consumer, no manual re-send. - On denial or dismissal the turn fails cleanly with the reason; a denial is recorded, a dismissal ("no answer, never consent") is not. The elicitation model can block a live SDK turn; skill expansion is pre-loop, so the turn is already dead by the time we know. Hence retry-a-fresh-turn rather than resume, triggered by the approval resolving. A guard prevents looping: a command that is already granted yet still blocks falls through to the terminal error instead of re-asking. Covered at both levels: provider unit tests for park→retry→success, deny, dismiss, and the no-loop guard; a session-integration test that approval_pending yields waiting_approval (not error) and surfaces the cue. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
🤖 Gemini code reviewAdds detection and recovery for blocked skill slash-command expansion failures (#232, #233), trust-gates pack skill symlinking, parks turns for human approval when skill commands are blocked, and fixes integration test setup. Findings: 🔴 0 · 🟠 1 · 🟡 0 · 🟢 0 Tokens spent · ⬆️ Input: 18,753 · ⬇️ Output: 276 · Σ Total: 25,153 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #235 +/- ##
=======================================
Coverage ? 88.51%
=======================================
Files ? 135
Lines ? 23570
Branches ? 0
=======================================
Hits ? 20864
Misses ? 2706
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…233) Gemini review: the argv0 executable-name check `/^[A-Za-z0-9_./~-]+$/` excluded `=`, so a command whose first token is an assignment (`FOO=1 ./run.sh`) was filtered out of skillCommandAllowRules. It would never be pre-authorized, and after a human approved it the retry re-derived the rule set — filtering it out again — so the grant was never applied and the turn failed permanently. Add `=` to the class. Comma/space-bearing prose (`suffix,`) is still rejected, so the anti-prose guard is intact. Test covers the env-prefixed case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the class of silent failures where a skill's slash-command expansion was blocked and the turn vanished — plus everything that surfaced once the pipeline could finally run far enough to expose it.
The original bug (#232)
A skill's
SKILL.mdmay contain!`…`shell substitutions that run at slash-command expansion time, before the agent loop. In a headless session those have no approval path, so a blocked one aborts the whole expansion and the SDK returnssubtype: "success", num_turns: 0— a turn that never ran, reported as success. Codeoid took it at face value: no output, no error, no retry.What this branch does
Detection (#232) — treat
num_turns === 0as an error regardless ofsubtype, attributed to the<local-command-stderr>that caused it. Keyed on the typednum_turnsfield (not string-matching), so it catches any consumed-but-unrun prompt.Permission (#233) —
options.allowedToolsgoverns the expansion-time check, so derive one verbatimBash(<cmd>)rule per substitution the installed skills declare and pass them in. No repo mutation, no hand-editedsettings.json. Measured the matching semantics (compound commands need the exact whole-string rule; wildcards would over-grant); two parsing guards (newline exclusion, executable-name check) found against the real skill set.Sandbox (#233) — running a command and reading the files it touches are independent gates. Pack skills are symlinks, so a substitution reading its own
templates//partials/resolves outsidecwdand is denied. Grant each skill's resolved parent viaadditionalDirectories.Human-in-the-loop (#233) — route approval through
requestUserInput(a real dialog), notcanUseTool— session mode expresses trust in the agent, not in code an installed pack runs unconditionally, and the tool path letautonomousauto-approve with no dialog. Verdicts persist per workspace + verbatim command; a denial is recorded, a dismissal never is.Pack trust (#233) — gate skill linking on pack trust, closing a declare-is-execute hole: an untrusted pack's
!…`` frontmatter was becoming runnable host shell with no opt-in.install()links only when trusted; `trust()` links on the ON transition.Park + retry (#233) — the big one. A blocked command now parks the turn (
waiting_approval) instead of failing it, so a driving pipeline phase stays alive. On approval the query loop rebuilds with the grant and retries the same prompt in place — no manual re-send, no dead pipeline. Denial/dismissal fails cleanly; an already-granted-but-blocked command falls through (no loop). Modeled on the existing elicitation pattern.Also here
fix(test): the ZeroID integration suite gated on/health(unauthenticated), so a reachable-but-unusable endpoint died mid-suite and cascadedundefined is not an objectinto 6 tests. Now probes the actual API and skips with the real reason. (Surfaced @highflame/sdkfix: sanitize untrusted output in the legacy readline terminal client (terminal/client.ts) #92 —AgentsResource.basePathmissing/api/v1, filed separately.)chore: ignore local.claude/worktree + personal settings state.Testing
Typecheck + lint clean; 1812 pass / 0 fail. Coverage includes: zero-turn detection, allow-rule derivation (+ prose/no-loop guards), sandbox symlink resolution, park→retry→success / deny / dismiss / no-loop, pack-trust link gating, and a session-integration test that
approval_pendingyieldswaiting_approval(not error).Related
Filed from this work: codeoid #232, #233, #234; highflame-sdk #92. Companion ai-factory fix (implement-phase pipeline nesting) merged as ai-factory#4.
🤖 Generated with Claude Code