The "does not gate as destructive" cases guarded the decision behind
`if (output && output.hookSpecificOutput)`, so a crashed or silent hook made
parseOutput return null and the test passed having asserted nothing.
Assert the exit code and that output parsed first, then branch: a decision
object must not be a Destructive deny, and pass-through must echo the input
back — the same shape the existing retry case already checks.
Raised in review on #2829.
Review on #2829 found the regex fix widened a false positive: matching
`\bdd\s+if=` against the whole flattened line gated `echo dd if=/dev/zero`
and `grep dd if=/dev/zero file`, neither of which runs dd. That class was
already present before — `echo dd if=x` matched the old arm too — but the
boundary fix extended it to the slash and dot spellings, so the arm now
decides on text position rather than on what is being executed.
dd moves to isDestructiveDd(tokens), next to isDestructiveRm and
isDestructiveGit, and DESTRUCTIVE_SQL_DD goes back to SQL only. The
per-segment loop already tokenizes every executable body, so the check runs
where the command word is known.
This resolves four things the text match could not:
dd if=/dev/zero of=/dev/sda was allowed -> denied (the reported bug)
sudo dd if=/dev/zero was allowed -> denied
dd of=/dev/sda if=/dev/zero was allowed -> denied (operands are order-free)
echo dd if=x was denied -> allowed (pre-existing false positive)
Leading sudo/doas/env, their flags, and VAR=value assignment prefixes are
skipped so a wrapped invocation still resolves to dd; flags are only skipped
once a wrapper has been seen, so the scan cannot walk into an unrelated
command's arguments.
Tests: 6 fail on upstream main, 155 pass with this change.
Refs #2642
DESTRUCTIVE_SQL_DD shared one trailing \b across every alternation arm.
`dd\s+if=` ends in `=`, and a \b after a non-word character only holds when
the NEXT character is a word character, so the arm matched `dd if=x` and
missed every path starting with `/`, `.` or a quote:
dd if=/dev/zero of=/dev/sda allowed
dd if=./disk.img of=/dev/sdb allowed
dd if="/dev/zero" of=/dev/sda allowed
The word-boundary suffix now applies only to the arms that end in a word
character. The split is what keeps the widening bounded: dropping the
trailing \b outright would let `truncate` match `truncated`, and dropping the
leading \b would let `dd if=` match inside `add if=`. Both are covered.
This is the half of #2642 that survived the structural findGitSubcommand()
parser, which already handles `git checkout -- .`.
Not addressed here: `dd of=/dev/sda if=/dev/zero` with the operands reversed
is still allowed, before and after, because the pattern requires `if=`
immediately after `dd`. That is a different defect from the boundary bug and
widening a P0 gate's pattern shape is a maintainer call.
Refs #2642
Keep npm and Yarn resolution policy aligned so both lockfile paths stay on the patched release.\n\nCo-authored-by: Svector-anu <svector-anu@users.noreply.github.com>
* docs(ito-compute): document ito accept and ito_accept MCP workflow
Updates the canonical ECC skill to cover the new quote acceptance path:
- CLI: ecc ito accept <ticket-id>
- MCP: ito_accept tool
- Explicit buyer-authority guard before accepting
- Clear statement that accept routes to desk, does not purchase
* test(ito-compute): assert the four-tool MCP boundary including ito_accept
The exact-boundary test pinned the three-tool description. Runtime
ito-compute-cli now exposes ito_accept (Ito-Markets/ito-cloud-runtime#1453),
so the template boundary assertion moves to four tools.
* docs(ito-compute): drop the firm-quote gate from the accept workflow
Desk quotes are indicative_paper in production (a firm quote requires the
separate human-held signing path and cannot reach the client), so gating
accept on 'a firm quote is ready' described an unfireable condition. Align
with the runtime contract: accept routes the current desk quote to human
review and the result carries quote_class (ito-cloud-runtime#1453).
The uninstall cases run two full CLI passes (install, then uninstall)
over several hundred files under a flat 30s timeout, which is tight
enough on Windows CI to fail intermittently with spawnSync ETIMEDOUT.
install-apply.test.js already scales its timeout by platform for the
same reason; match that precedent rather than re-running past the flake.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The guided installer asks how ECC hooks should run, but that consent
lived only in the wizard path. Running install-apply directly with a
profile that includes hooks-runtime still materialized the hook runtime
with no disclosure and no decision.
Gate the apply layer instead, so every entry point is covered:
- disclose the six hook capability groups when a plan would materialize
the hook runtime, and refuse to apply until the caller decides
- --enable-hooks confirms the hook runtime; --no-hooks installs the rest
of the selection without it and records the reduced module closure in
install-state
- surface the pending decision as a dry-run warning
- show the same capability disclosure in the guided installer's plan
preview, so the wizard's hook question states what it is asking about
Plans that never materialize hooks (Kimi, --profile minimal,
--without baseline:hooks) are unaffected and need no flag. Repair and
uninstall operate on already-recorded state and stay unchanged.
The capability taxonomy and the held-materialization behavior come from
Samarjeet Singh Tomar's PR #2634, reworked to fit the single-decision
consent model that shipped with the guided installer in #2649.
Co-Authored-By: Samarjeet Singh Tomar <samar_tomar@hotmail.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
_parse_stream_json() persisted raw tool_input/tool_response content into
ObservationEvents that grade() scores and generate_report() writes to
results/<skill>.md -- a report meant to be shared and reviewed.
--add-dir restricts the agent's additional accessible directory to the
sandbox (SANDBOX_BASE = /tmp/skill-comply-sandbox), but that doesn't stop
the agent's own tool calls (a Bash command using ~ expansion, a scenario
setup_commands entry referencing a dotfile) from emitting the operator's
home directory into tool_input/tool_response -- which then lands
verbatim, truncated but not sanitized, in the written report.
Adds _redact_home_path(), pure stdlib (Path.home()), applied to both
input_str and output_str before they're stored on the ObservationEvent.
Scoped deliberately to the home directory only -- grade() needs real
tool-call semantics for LLM-based compliance classification, so
truncating/stripping content the way a pure logging hook could isn't an
option here; only the operator-identifying path component needs to go.
New TestParseStreamJsonRedactsHomePath class in
skills/skill-comply/tests/test_runner.py (3 tests) -- full file now
10/10 passing, up from 7/7. Confirmed tests/test_invariant_runner.py (the
sandbox-execution security tests from #2149) still passes clean, 4/4.
Fixes#2730
The find -> sort -> read chain in both scripts used newline-delimited
records (plain read -r), so a skill directory name containing a
literal newline would be split across two records. Verified with a
directory literally named "evil\nskill": the old reader produced a
truncated "evil" fragment plus an orphan "skill/SKILL.md" fragment,
inflating the skill count and throwing awk/date errors on the garbage
paths.
Switch to -print0 / sort -z / read -r -d '' in both scripts so a path
is always read as a single record, regardless of its contents. Paths
under scan are untrusted input.
Addresses further CodeRabbit review feedback on PR #2640.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FhDjpSfrbPpnpqT3CZBEX1
Nguyen Thanh Dat