Repository navigation
Close the review gaps in the Claude adapter gates - #19
Conversation
The withheld-tool gate scanned only skills/ and accepted any line that contained "do not call" somewhere, so an instruction to call the tool in a later clause, or in README or a slash command, passed. It now reads every Markdown and YAML file the adapter ships and checks each mention: the sentence must prohibit, and the mention must not be the direct object of an un-negated call verb. load_claude_overlay rejects a duplicate YAML key (PyYAML keeps the last one and drops the rest of the block silently) and two spellings of one edit path that normalise onto each other. The sent-feedback eval case that asks for help with a scheduled send is a hard negative under the no-solicitation policy, not ambiguous. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| WITHHELD_PROHIBITION = re.compile(r"\b(?:do not|don't|never|cannot|can't|not)\b", re.IGNORECASE) | ||
| WITHHELD_CALL_VERB = re.compile(r"\b(?:call|calls|calling|run|runs|running|invoke|invokes|use|uses|execute|executes)\s*`?\s*$", re.IGNORECASE) | ||
| WITHHELD_NEGATED_CALL = re.compile( | ||
| r"\b(?:do not|don't|never|cannot|can't|not)\s+(?:\w+\s+)?(?:call|calls|calling|run|runs|running|invoke|invokes|use|uses|execute|executes)\s*`?\s*$", | ||
| re.IGNORECASE, | ||
| ) | ||
|
|
||
|
|
||
| def withheld_tool_mentions_are_prohibitions(line: str, tool: str) -> bool: | ||
| """True when every mention of ``tool`` on ``line`` is governed by a do-not-call instruction. | ||
|
|
||
| Sharing a line with the phrase is not enough: "do not call `x`; otherwise call `x` with the plan" | ||
| instructs the call in its second clause. Each mention must sit in a sentence that prohibits | ||
| something, and must not be the direct object of an un-negated call verb. | ||
| """ | ||
| for sentence in WITHHELD_SENTENCE_BOUNDARY.split(line): | ||
| for match in re.finditer(re.escape(tool), sentence): | ||
| if not WITHHELD_PROHIBITION.search(sentence): | ||
| return False | ||
| preceding = " ".join(sentence[:match.start()].split()[-4:]) | ||
| if WITHHELD_CALL_VERB.search(preceding) and not WITHHELD_NEGATED_CALL.search(preceding): |
There was a problem hiding this comment.
🔴 Adapter text that tells Claude to call a withheld tool now passes the gate, even though the base gate rejects it. A bare "not" or "never" anywhere in the sentence counts as a prohibition (line 703). Only six call verbs, directly before the tool, count as an instruction. The one-word gap in WITHHELD_NEGATED_CALL also treats "Do not skip calling x" as negated. Fix: keep the base requirement that each mention's sentence contains "do not call", then apply the new checks on top. Allow no filler word between the negation and the verb. All three current mentions still pass under this fix. [also at: scripts/validate.py:724 - Adapter text that tells the agent to use a withheld tool now passes the gate, as long as the sentence has a bare "not" and no call verb right before it.]
Why this was flagged
Trigger: an overlay edit, or a file shipped in claude-plugins/sent, adds a line such as "If no profile exists, it is not optional: create one with sender_profiles.create.", "Never skip sender_profiles.create." or "Do not skip calling sender_profiles.create.". The gate runs in validate_claude_adapter (scripts/validate.py:693-698). WITHHELD_PROHIBITION (line 703) matches the bare "not"/"never". The words before the mention are "create one with" or "Never skip", and WITHHELD_CALL_VERB (line 704) does not match either. For "skip calling", WITHHELD_NEGATED_CALL (line 706) uses (?:\w+\s+)? to treat "not skip calling" as negated. So withheld_tool_mentions_are_prohibitions returns True and nothing is reported. On the base branch, any line naming a withheld tool without the literal "do not call" was rejected, so all three lines failed validation. After the change they ship to Claude. The new test cases only cover call/run/invoke/use phrasing, so they do not catch this. The PR describes the change as a stricter gate.
Verification: The base gate flagged any line naming a withheld tool unless the line contained "do not call". The new code in scripts/validate.py drops that requirement. WITHHELD_NEGATED_CALL allows one filler word, (?:\w+\s+)?, between the negation and the verb. This is lost safety coverage: adapter text that tells Claude to create a sender profile now ships without an error.
| def construct_mapping(self, node, deep=False): # type: ignore[override] | ||
| seen: set[Any] = set() | ||
| for key_node, _ in node.value: | ||
| key = self.construct_object(key_node, deep=deep) |
There was a problem hiding this comment.
🟡 (optional) Maintainers who use a YAML merge key (<<: *anchor) in overlay.yaml will now get a load failure; the base yaml.safe_load accepted it. The duplicate check calls construct_object on every raw key node before flatten_mapping runs, which includes the << node tagged tag:yaml.org,2002:merge. SafeLoader has no constructor for that tag, so loading fails with "could not determine a constructor for the tag", which does not point to the merge key. Fix: skip key nodes tagged tag:yaml.org,2002:merge in the duplicate scan and check only explicit keys. Merged keys that explicit keys override are legal YAML and must still load. [also at: scripts/repository_metadata.py:170 - nit: A malformed overlay with a non-scalar mapping key now crashes with a raw TypeError traceback instead of a clean MetadataError.]
Why this was flagged
The trigger is an overlay.yaml that uses a merge key, for example an edit entry written as - <<: *common plus find:. No such entry exists today, so this is conditional on a maintainer adding one. UniqueKeyLoader.construct_mapping (repository_metadata.py:163-164) loops over node.value and calls self.construct_object(key_node) for every key. It does this before super().construct_mapping runs flatten_mapping, the step that removes merge-key nodes. The << scalar resolves to tag:yaml.org,2002:merge. SafeConstructor registers no constructor for that tag, so construct_undefined raises a ConstructorError. repository_metadata.py:176-177 turns that into MetadataError "could not read adapter-sources/claude/overlay.yaml: ... could not determine a constructor for the tag 'tag:yaml.org,2002:merge'". On the base, yaml.safe_load flattens merge keys and loads the file. After this change, generation, validation and test_contracts all fail on a valid overlay, with an error that does not mention a merge key.
Verification: Trigger: a maintainer adds a YAML merge key (<<: *anchor) anywhere in adapter-sources/claude/overlay.yaml. The current file has none. In scripts/repository_metadata.py:161-170, construct_object runs on every key before return super().construct_mapping(node, deep=deep), and only the super call runs self.flatten_mapping(node). Base yaml.safe_load flattened merge keys and accepted the same file.
…merge keys Review on #19: a bare "not" or "never" anywhere in the sentence counted as a prohibition, so "it is not optional: create one with `x`" and "Never skip `x`" passed, and one filler word was allowed between the negation and the verb, so "Do not skip calling `x`" passed too. The sentence must now contain do-not/don't/never directly on a call verb, and the per-mention object check applies on top. The three real mentions pass. The duplicate-key loader now skips merge-key and non-scalar key nodes, which the parent flattens or reports, instead of failing to construct them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
alban-sent
left a comment
alban-sent
left a comment
There was a problem hiding this comment.
Reviewed the validation scope, per-mention prohibition checks, duplicate-key handling, normalized path collision detection, and regression coverage. The follow-up commit addresses the earlier gate bypass and YAML merge-key concern. CI passes, and I found no remaining merge blockers.
3 participants
Close the review gaps in the Claude adapter gates
Follow-up to #18. The automated review left three optional findings on the overlay machinery. All three were real gaps; none changed the shipped 0.3.0 output.
Changes / Fixes / Features included
scripts/validate.py: the withheld-tool gate reads every Markdown and YAML file the Claude adapter ships (README,.claude/commands/*.md, skills and their references,agents/*.yaml), not onlyskills/. Each mention of a withheld tool must sit in a sentence that prohibits something, and must not be the direct object of an un-negated call verb.do not call x; otherwise call x with the planfails now. The three real mentions in the adapter still pass.scripts/repository_metadata.py:load_claude_overlayrejects a duplicate mapping key anywhere inoverlay.yaml, where PyYAML silently keeps the last one, and two spellings of one edit path that normalise to the same target, such as./skills/xandskills/x.evals/sent-feedback.yaml: the scheduled-send case that asks for help rather than a report isno_triggerunder the no-solicitation policy. A correctno_triggerprediction counted as a misroute before. Oneambiguouscase remains, asvalidate_evalsrequires.scripts/test_validation_gates.py: five new rejection cases, one per gap plus the README and slash-command scopes.Verification
generate_adapters.py --checkvalidate.pytest_validation_gates.pytest_fixtures.py,test_contracts.pyHow to test?
python scripts/validate.pyandpython scripts/test_validation_gates.py.Otherwise call \sender_profiles.create` with the plan.to the sent-compliance edit inadapter-sources/claude/overlay.yaml, regenerate, and runvalidate.py. It fails with "outside a do-not-call instruction". Onmain` it passes.edits:in the overlay and runvalidate.py. It fails with "found duplicate key".🤖 Generated with Claude Code