tools/list and tools/call on main already admit `_meta` — MCP reserves it
for request metadata and a client may attach it to any request. ping still
refused every parameter, so a client that sends `_meta` on everything
(Codex does) got -32602 on its keepalive.
Rebased onto main and narrowed: when this branch was first written the same
gap existed on tools/list, which has since been fixed upstream. Only the
ping handler is left, so only the ping handler is touched.
Refs #2810
Named in review on #2829: `dd bs=1M if=/dev/zero of=/dev/sda` is the same
class as the reversed-operand case and is denied by the token-based check,
but nothing pinned it.
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
Review feedback on #2878. stdout is async when it is a pipe, which is exactly
how tests/run-all.js runs these files, and process.exit() does not wait for
pending writes — so exiting that way can drop the Passed:/Failed: lines the
aggregator totals, defeating the previous commit. The sibling
tests/ci/ito-*-skill.test.js files already use process.exitCode.
Still exits 1 on a broken counter (Passed: 4, Failed: 7) and 0 when clean.
Review feedback on #2879. stdout is async when it is a pipe, which is exactly
how tests/run-all.js runs these files, and process.exit() does not wait for
pending writes — so exiting that way can drop the Passed:/Failed: lines the
aggregator totals, defeating the point of printing them. The sibling
tests/ci/ito-*-skill.test.js files already use process.exitCode.
Still exits 1 on drift and 0 when clean.
Review feedback on #2879, citing rules/common/coding-style.md ("Immutability
(CRITICAL): ALWAYS create new objects, NEVER mutate existing ones"). Build one
list of locale entries, then derive the matched and orphaned sets from it with
filter/map, and build the canonical Map the same way.
Behaviour is unchanged and both mutations still fail: reintroducing the drift in
one locale file gives `Passed: 4, Failed: 3`, an orphan gives `Passed: 6,
Failed: 1`, both exit 1.
Review feedback on #2879: a locale file with no counterpart in `agents/` was
skipped, so every other case here silently passed over it. Retiring an agent
would leave its seven translations behind with nothing to compare against and
nothing to report it.
Collect those files while building the list and assert the collection is empty.
Zero today across 199 locale docs, so this changes no current result; planting
one orphan takes the file to `Passed: 6, Failed: 1` and exit 1.
Review feedback on #2878: the static assertions ran at the top level, so a
failure exited the process before the `Passed:`/`Failed:` lines. tests/run-all.js
totals those tokens, so the per-case counts were lost — it still went red via the
non-zero exit, but the granular numbers were not in the totals.
Route every case through the same `runTest()` wrapper the file already used for
the integration cases, and build the case list inside a try so a missing or
renamed `instinct-cli.py` / `start-observer.sh` is a reported failure rather than
a crash.
Reverting the fix now prints `Passed: 4, Failed: 7` and exits 1, naming all seven
broken expectations instead of stopping at the first.
`scripts/ci/validate-agents.js` reads only `agents/`, so the translated copies
under `docs/<locale>/agents/` were never validated against the agent they
describe — and drifted.
Two kinds of drift, both machine-checkable and both wrong in the same
direction (the translations were made from an older revision and never
re-synced):
- **Model tier, 39 files.** Every one names a costlier tier than ships:
haiku -> sonnet, sonnet -> opus. `security-reviewer` reads `opus` in ja-JP
and zh-TW; it ships `sonnet`.
- **Tool set, 15 files.** `security-reviewer` and `database-reviewer` list
`Write` and `Edit` in **all seven locales** — both agents ship read-only
(`Read, Grep, Glob, Bash`). `seo-specialist` in zh-CN adds a `Bash` the
canonical agent does not have. Documenting a reviewer as able to write is
the kind of inaccuracy someone auditing what these agents can touch would
act on.
Sync `model` and the `tools` set to canonical, preserving each locale's
existing list style so the diff is only the values that were wrong. The
`["a", "b"]` vs `a, b` formatting difference is left alone: it is consistent
per locale and carries no meaning.
Add `tests/ci/locale-agent-frontmatter.test.js` so this cannot drift back.
`tools` is compared as a set, not a string, so the style difference stays
legal; prose is not compared at all. The last case pins the specific failure:
a canonical read-only agent may never be documented with Write or Edit.
`.kiro/agents/` is deliberately excluded — it uses a different schema
(`allowedTools: [read, shell]`, no `model`), not a translation of this one.
`start-observer.sh status` globbed `*.yaml`, but the observer prompt tells the
analyzer to write `${INSTINCTS_DIR}/<id>.md` and the loader accepts
`.yaml`, `.yml`, and `.md` (ALLOWED_INSTINCT_EXTENSIONS in
scripts/instinct-cli.py). The one command an operator runs to confirm that
learning works therefore reported `Instincts: 0` on a healthy install — which
is indistinguishable from a silently dead observer, exactly the failure the
status check exists to surface.
Match the loader instead of one of its three extensions, and match how it
enumerates them: `Path.iterdir()` is top level only and `is_file()` skips
directories, so `-maxdepth 1 -type f`; `suffix.lower()` makes the comparison
case-insensitive, so `-iname`. `tr` drops the column padding BSD `wc` emits,
which is why the reported output read `Instincts: 0`.
Verified end to end against the shipped script with 3 `.md`, one `.yaml`, one
`.yml`, one `.YAML`, a `notes.txt`, and a nested `.md`: 1 before, 6 after —
the same six files the loader picks up.
Stylelint resolves stylelint.config.{ts,mts,cts} through cosmiconfig and
markdownlint reads .markdownlint.{cjs,mjs}; both were still editable while the
hook was active.
The hook blocks edits to linter/formatter configs so an agent fixes the code
instead of weakening the checks. It did not cover the cheapest way to weaken
them: adding one path to an ignore file. Measured against the hook, with the
file already on disk:
.eslintignore -> allow
.prettierignore -> allow
.stylelintignore -> allow
.markdownlintignore -> allow
Nor did it cover the current config names — only the legacy `.stylelintrc*`
and `.markdownlint.json` spellings were listed, so a project using the
documented `stylelint.config.js`, `.markdownlint.jsonc` or markdownlint-cli2
had no protection at all.
First-time creation stays allowed by the existing existence check, so
scaffolding a fresh ignore file is unaffected.
Nguyen Thanh Dat