Skip to content

feat(architecture): concise skill text measured by evals, plus outcome cases and probes - #7437

Merged
kyle-sexton merged 11 commits into
mainfrom
feat/architecture-concise-skills
Oct 14, 2026
Merged

kyle-sexton merged 11 commits into
mainfrom
feat/architecture-concise-skills

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Oct 14, 2026 •

Copy link
Copy Markdown
Member

Refs: #6551
Refs: #6476

Summary

This is the architecture part of the concise-skills rollout. It was done with the per-plugin runbook (/playbooks:skill-authoring). The protocol was the same as the other rollout PRs:

  • before and after, on both Opus and Sonnet
  • each outcome case run 3 times, graded by deterministic graders
  • held-out trigger queries run once each
  • --ablation none, and --max-cost-usd 5 on every pass

What changed:

  • Descriptions: all eleven model-invoked skills have shorter descriptions. Every quoted trigger phrase and every "Skip when" route is kept. setup is user-invoked, so it is untouched.
  • Bodies: three are tighter: map-data, map-deployment and map-flow. The other eight keep the main text, for the reason below.
  • Ten new outcome cases, one per map-* skill plus one for record-decision. Frozen trigger-probe sets for all eleven skills, under probes/. Stricter graders on the existing improve cases. No run artifacts are committed.

This PR leaves #6551 open. Merging it should close the issue only if the owner agrees that "eight bodies stay at main" meets the issue's done-when.

Fix

Why eight bodies stayed at main

The first rewrite cut every body. A fresh-context meaning diff of each body against main then found real semantic losses. These included dropped "one command per call" rules, dropped "does NOT do" bullets, dropped reasons and dropped gotchas.

Every loss was put back verbatim. After that, eight bodies differed from main by less than 1% of their words. That is mostly reflow and punctuation, not worth an outcome measurement or a review, so those eight bodies were reset to main exactly.

The three bodies that kept a cut each got a second fresh-context meaning diff against main. All three returned PASS, with no loss and no ambiguity.

Per-skill decision table

Skill Description chars (main → shipped) Body lines, words (main → shipped) Decision
improve 546 → 533 97, 1,297 → unchanged description new; body main
map-components 463 → 452 275, 2,096 → unchanged description new; body main
map-containers 464 → 453 296, 2,768 → unchanged description new; body main
map-context 463 → 461 233, 1,973 → unchanged description new; body main
map-data 482 → 475 248, 2,320 → 246, 2,307 new
map-dependencies 461 → 434 258, 2,196 → unchanged description new; body main
map-deployment 373 → 366 478, 4,825 → 470, 4,761 new
map-events 415 → 406 142, 1,161 → unchanged description new; body main
map-flow 486 → 470 230, 2,146 → 227, 2,118 new
map-landscape 483 → 469 268, 2,441 → unchanged description new; body main
record-decision 474 → 464 145, 1,192 → unchanged description new; body main

Outcome suite

Before = main body; after = the shipped body. Each case ran 3 times per arm on each model, with both arms launched together, against the frozen suite.

Case (skill) Sonnet before Sonnet after Opus before Opus after Verdict
md-prisma-sql-mismatch (map-data) 1.00 1.00 1.00 1.00 kept
mf-interface-hop-unresolved (map-flow) 1.00 1.00 1.00 1.00 kept
my-diff-secret-redacted (map-deployment) 1.00 1.00 1.00 1.00 kept

Each kept cut sits in a section its case reaches:

  • the tier/mismatch rules for map-data
  • the receiver-type and hand-off rules for map-flow
  • the redaction and diff rules for map-deployment

No paired run was needed for the eight cases whose skill body equals main (mc, me, mk, ml, mp, mx, rd, and the four improve cases), because their before and after arms are identical.

Triggering (held-out)

There were 6 queries per skill (4 that should trigger, 2 that should not), 66 in all. They were written blind, run once each, and are not committed.

Model Before positives After positives Before negatives quiet After negatives quiet
Opus 43/44 44/44 22/22 22/22
Sonnet 38/44 38/44 22/22 22/22
  • On Sonnet, the same six positives miss in both versions: improve pos-1/3/4, map-dependencies pos-4, map-deployment pos-4 and map-flow pos-4.
  • On Opus, the one miss is map-flow pos-4, and only in the before version.
  • No skill lost a query.
  • The Sonnet runs come from an earlier session. They are reused because the descriptions and queries have not changed since. Each Opus pass hit the 5 USD ceiling, so the missing cases were re-run by glob, both versions together.

Verification

  • Grader audit before the freeze. Each fix has must-pass and must-fail samples.
    • mc gains no-components-shell-write. It catches cp, mv, install, dd, sed -i, tee or a redirect into components.md, and any pipe to a shell.
    • ml gains no-shell-write. It catches the same write forms into an artifact file, plus mkdir, touch, rm and pipe-to-shell. /dev/null and /tmp/ are exempt.
    • rd no-shell-write now also catches cp, mv, install, dd of=, sed -i, redirects to .txt, .adoc and .rst, and pipe-to-shell.
    • md reply-reports-mismatch, mf reply-reports-di-hop and mp reply-names-unresolved reject "not one …" and "not unresolved".
    • improve says-already-deep rejects "not already deep".
    • Both states-no-candidates graders no longer match an unrelated "no" ("No problem, here are two candidates").
  • Changes after the freeze. None changes what a grader accepts. Each change refroze the digest. Final digest: 4e25ec3a….
    • The pipe-to-shell alternative is spelled (?:bash|zsh|dash|sh) instead of (?:ba|z|da)?sh, to satisfy typos. The two match the same strings.
    • The fixture secrets in mk, mx and my are now CANARY-… markers instead of credential-shaped strings. my was re-run on both models in both arms after the change, and every arm scored 1.00. mk and mx passed a 3-run Sonnet smoke.
  • Every outcome pass ran with --scaffold --allow-tools Bash Write Edit, so the tool_used … max: 0 graders could fail.
  • Ready-time reviews, run as fresh-context subagents over the merged head:
    • Security raised one P4. GitGuardian flagged the Password= / Token= / DB_PASSWORD: canary fixtures as generic secrets. The mx and my scaffolds now assemble those values at run time, and the output is byte-identical to before (diffed). The no-secret-* fail samples no longer carry the key text. A local gitleaks scan of plugins/architecture is clean. Security also raised one P5: the ml fixture names a real GitHub owner that no expected path ever contacts. That one is left as is.
    • Code raised two P5s on map-deployment, both fixed here. The rewrite had added a body sentence that repeats a gotcha and spelled out Terraform's tfvars precedence without a stamp. Both now carry main's text, and two lines were reflowed. These were the only body changes made after the measured runs. Each restores main wording, so the shipped body sits between the two measured arms.
    • Code also raised ten grader findings on frozen graders, and one note on missing-file not_contains passes. They are filed with every quoted input as test(architecture): tighten eval graders the #7437 ready review found lenient #7438. In the measured cases, only md and mf are touched. Every run of both scored 1.00 and wrote the expected files.
  • Earlier smoke. A Sonnet smoke of every case on main bodies, one run each before the grader tightening, passed all ten new cases.
  • Static checks:
    • /evals:validate: no FAIL. The warnings are the expected --allow-tools grants and judge-calibration notes.
    • measure-invocation.sh validate plugins/architecture/probes: PASS, 0 warnings.
    • check-skill.sh plugins/architecture/skills: every skill PASS except map-landscape. Its render-landscape.test.sh markdownlint case fails on main too, with that skill's body unchanged here.
    • typos and markdownlint are clean. check-changelog-fragments.sh --check, check-orphaned-fixtures.sh --check and check-fixture-git-isolation.sh pass. Scaffolds are committed executable.
  • Known gaps:
    • Haiku and Fable are untested.
    • Triggering has no plugin-vs-no-plugin delta.
    • The meaning diffs are model judgments.
  • Eval spend (list-price estimates): 39.71 USD in this session (27.21 USD Opus triggering, 12.50 USD outcome and smoke), plus 13.73 USD of Sonnet triggering and smoke in the earlier session.

Related

🤖 Generated with Claude Code

kyle-sexton and others added 2 commits October 14, 2026 00:21
…e cases and probes

Shorter descriptions for the eleven model-invoked skills and tighter
bodies for map-data, map-deployment and map-flow; the other bodies keep
the main text. Ten outcome eval cases (one per map-* skill and
record-decision), frozen trigger-probe sets for all eleven skills, and
stricter improve graders.

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@gitguardian

gitguardian Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

…map-deployment wording

The mx and my scaffolds assemble their fake secret values at run time so
secret scanners do not flag them; the generated files are unchanged. The
no-secret fail samples drop the key text. map-deployment keeps main's
text for the CloudFormation pairing note and the Terraform resolution
gotcha.

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 14, 2026 04:31
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-14T04:35:35.287040Z 9c76a3d Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 3m 17s —— View job


Security review complete

  • Gather context (PR diff, changed files)
  • Review eval fixtures/scaffolds for secret-handling and shell-injection risk
  • Review grader/skill text changes for instruction-surface deletions
  • Check canary-fixture run-time assembly (mx/my scaffolds) per PR description
  • Adversarial verification of candidate findings
  • Post final results

No security issues found.

This PR is almost entirely eval scaffolding/fixtures, probe sets, grader regex tightening, and skill-body reflow/wording changes (no production script logic changed). I checked the areas most likely to carry risk:

  • Secret handling in fixtures. mk, mx, and my scaffolds (plugins/architecture/evals/{mk-shared-broker-redacted,mx-actors-stated-only,my-diff-secret-redacted}/scaffold.sh) use CANARY-... markers, and mx/my assemble the password/token key names and values at run time (string concatenation / printf) rather than embedding credential-shaped literals, matching the PR description's stated GitGuardian fix. No non-canary secret-shaped strings found across any new scaffold.sh, case.yaml, prompt.md, or samples/*.json.
  • Shell injection in scaffolds. All new scaffold.sh files use set -euo pipefail, heredocs with hardcoded content, and no eval, curl, wget, command substitution of untrusted input, or path traversal outside the sandbox. They're self-contained fixture generators, not processing external input.
  • Instruction-surface deletions (per this skill's protected-class lens) in map-data, map-deployment, and map-flow SKILL.md bodies. Diffed each against origin/main word-for-word: every "does NOT do" bullet, refusal rule, and redaction/diff rule is preserved verbatim; changes are reflow/punctuation only. One bullet in map-deployment's Gotchas ("A Compose layer with no declared merge order is refused by name, never guessed") was removed from the prose, but the underlying refusal logic is still enforced by the unchanged collect-deployment.sh script (confirmed via COMPOSE_FILE/merge-order handling at lines 30-31 and 452/470) — no script files changed in this PR — so per the register's own test ("nothing else in the tree still enforces it") this is a doc-only trim, not a guardrail removal.
  • Eval tool grants. --allow-tools Bash Write Edit is scoped to the eval harness's sandboxed scaffold runs, consistent with existing cases; no case grants anything broader.

claude-lane-summary: lane=claude-security-review sha=9c76a3d7aa1c1a7a6c608d7ddd28a3da70ebc42e run=38511850024

@claude

claude Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Code review in progress

  • Gather context (CLAUDE.md, REVIEW.md, standards, PR diff)
  • Review skill/body/description changes
  • Review new eval cases, scaffolds, graders
  • Review probes and changelog fragment
  • Adversarially verify candidate findings
  • Post inline comments / summary

View job run

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9c76a3d7aa

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/architecture/evals/rd-no-convention-writes-nothing/case.yaml Outdated
@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

claude-security-review has reviewed this pull request through 74a1c24; a later push is reviewed from there.

…leaks

The branch's first commit carried the canary values inline; the later
commit assembles them at run time. Pin the six findings to that commit.

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 0s —— View job


Security review complete

  • Gather context (incremental diff, custom instructions)
  • Invoke /review:security-review
  • Review the .gitleaksignore change
  • Adversarially verify candidate findings
  • Post final results

Scope: incremental — only .gitleaksignore changed since this lane's last review of 9c76a3d. All other files were reviewed then.

No security issues found.

The diff adds six .gitleaksignore entries, each pinned to the specific historical commit de1c326 (the pre-fix commit GitGuardian flagged), file, rule, and line — the standard narrow commit:file:rule:line gitleaks-ignore format. I verified each referenced line against that commit's blob content:

Every flagged value is an explicitly-labeled CANARY-* fixture string, not a real credential, and the as-of-020b2e0 head already assembles these fixture values at run time (per the PR's security-fix description) rather than embedding them as literals. Each ignore entry is scoped to the one historical commit/file/line that carried the literal, so it does not suppress detection of any new secret introduced in these files going forward — this is a correctly narrow remediation of a historical false positive, not a weakening of secret-scanning coverage.

claude-lane-summary: lane=claude-security-review sha=020b2e0d3b05751e43f8d91e0e045fded3216467 run=38512239540

@claude

claude Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 4m 48s —— View job


Code review complete

  • Gather context (CLAUDE.md, REVIEW.md, standards, PR diff via .claude-lane/pr.diff)
  • Invoke /review:code-review per custom instructions
  • Review skill/body/description changes (all 11 SKILL.md diffs)
  • Review new eval cases, scaffolds, graders
  • Review probes and changelog fragment
  • Adversarially verify candidate findings
  • Post inline comments / summary

This repo runs a dedicated security lane (pr-review-security-hosted.yml), so this review is scoped to correctness/design/conventions per REVIEW.md; security findings (secret handling, canary assembly) are left to that lane, which already passed on this head.

SKILL.md changes. Diffed all 11 bodies (improve, map-components, map-containers, map-context, map-data, map-dependencies, map-deployment, map-events, map-flow, map-landscape, record-decision) against origin/main. map-data, map-deployment, and map-flow match the PR's claim: every bullet, refusal rule, and script-contract line survives, changes are reflow/punctuation and bullet reordering only (e.g. map-deployment's three Terraform/.tf.json Gotchas bullets moved position but are byte-identical in content). The other eight are description-only diffs as described.

3 findings posted inline:

  1. mp-unresolved-not-matched/case.yaml:21 — the reply-names-unresolved grader's two alternatives protect against negation asymmetrically; the unresolved ... path ordering is missing the (?<!n't\s) lookbehind the path ... unresolved ordering has, so a reply like "it isn't unresolved — the edge matched ../Shared/Shared.csproj" would incorrectly grade as a pass.
  2. mc-two-deployables-stops/case.yaml:34 — same interpreter-write gap Codex flagged on rd-no-convention-writes-nothing, recurring here: a python3 -c '...write_text(...)' Bash call bypasses the no-components-shell-write command allowlist.
  3. ml-check-writes-nothing/case.yaml:34 — same gap a third time, on no-shell-write.

All three are in net-new eval fixtures (not pre-existing), scoped to grader correctness rather than production code; no other correctness issues found in the scaffolds, probes, or changelog fragment.

claude-lane-summary: lane=claude-review sha=020b2e0d3b05751e43f8d91e0e045fded3216467 run=38512239502

Comment thread plugins/architecture/evals/mp-unresolved-not-matched/case.yaml Outdated
Comment thread plugins/architecture/evals/mc-two-deployables-stops/case.yaml Outdated
Comment thread plugins/architecture/evals/ml-check-writes-nothing/case.yaml Outdated
@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

claude-review has reviewed this pull request through 74a1c24; a later push is reviewed from there.

Shell-write graders now catch interpreter writes (python3 -c, node -e),
quoted targets, perl -i and >| (ml), and more extensions (rd). The mp
reply grader adds the missing n't guard plus a not_contains grader for
negated unresolved claims. Each fix has must-fail samples.

Refs #7438

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 7m 48s —— View job


Code review complete

  • Gather context (incremental diff .claude-lane/incremental.diff, custom instructions, validate-cases.py grader mechanics)
  • Invoke /review:code-review for the incremental scope
  • Review changed grader/case/sample files (mc, ml, mp, rd)
  • Adversarially verify candidate findings (manual regex trace, since python3/node execution is denied under this lane's permission mode)
  • Post inline comments / summary

Scope: incremental — only the 9 files this lane's instructions named, as diffed since 020b2e0. These are fixes for gaps the previous review rounds (Codex + this lane) flagged: missing interpreter-write detection in mc/ml/rd's no-shell-write graders, and the missing n't guard in mp's reply-names-unresolved.

1 finding posted inline:

  1. mc-two-deployables-stops/case.yaml:34 — the new python3/node alternative in no-components-shell-write has no write-action requirement, unlike the equivalent fixes in ml-check-writes-nothing and rd-no-convention-writes-nothing (both require a write/dump/mode-flag token, and both ship a read-only pass fixture proving it). A pure read like python3 -c "print(open('docs/architecture/components.md').read())" would be wrongly flagged as a forbidden write, since this case's Bash tool is unrestricted and reads of components.md are expected/allowed.

What I verified and found sound:

  • mp-unresolved-not-matched/case.yaml — the symmetric (?<!n't\s) guard added to reply-names-unresolved's first alternative correctly blocks the previously-flagged "isn't unresolved — the edge to ../Shared/Shared.csproj…" false pass (traced char-by-char: the 4-char fixed-width lookbehind lands exactly on "n't ").
  • The new reply-unresolved-not-negated backstop grader and its new sample file — traced all 2 pass / 3 fail cases against all 3 pattern alternatives; each classifies correctly, including the "Unresolved: none" and "isn't unresolved" phrasings the primary grader's window-based design can miss.
  • ml-check-writes-nothing and rd-no-convention-writes-nothing's new python3/node, perl -pi, quoted-target, and extension-list additions — traced each new fail/pass sample against the updated pattern; all match as intended, no false positives on the added read-only fixtures.

Known limitation on this verification: this lane's permission mode denies all python3/node execution (confirmed: even python3 -c "print(1)" requires approval), so these regexes could not be checked against a real JS/Python regex engine — the above is a manual, character-by-character trace against Python's re semantics (which validate-cases.py itself uses for offline grading) rather than an executed test.

claude-lane-summary: lane=claude-review sha=c7acfae5b3ab18a101d0fbc11085af47566f90d6 run=38513337764

@claude

claude Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 43s —— View job


Security review complete

  • Gather context (incremental diff, custom instructions)
  • Invoke /review:security-review
  • Review the changed grader/sample files
  • Adversarially verify candidate findings
  • Post final results

Scope: incremental — this lane previously reviewed 020b2e0. Only the 9 files listed in the lane's custom instructions changed since then (all in plugins/architecture/evals/{mc-two-deployables-stops,ml-check-writes-nothing,mp-unresolved-not-matched,rd-no-convention-writes-nothing}/), per .claude-lane/incremental.diff.

No security issues found.

This diff tightens four eval graders' tool_used/regex patterns and adds matching fixture samples, closing the exact gaps the prior review rounds (Codex + this lane's own claude-review) flagged on 020b2e0:

  • mc no-components-shell-write and ml no-shell-write now also match quoted/escaped targets, perl -pi, >| clobber, and python3 -c / node -e interpreter writes (mc/case.yaml#L34, ml/case.yaml#L34).
  • rd no-shell-write adds the same interpreter-write coverage plus .markdown/.yaml/.yml extensions (rd/case.yaml#L23).
  • mp reply-names-unresolved adds the missing (?<!n't\s) lookbehind to its first alternative, closing the asymmetric-negation gap, and a new reply-unresolved-not-negated grader adds a not_contains backstop for "nothing/none...unresolved" and "not/isn't unresolved" phrasing (mp/case.yaml#L21).

Findings on these changes:

  • No secrets or injection surface. All new content is regex patterns and static JSON fixture strings (fake paths like docs/architecture/components.md, docs/adr/0001-postgres.md); no credential-shaped or canary strings were touched.
  • No ReDoS risk. The new (?:[^"\\]|\\.)*? chains and interpreter-write alternatives are unambiguous character-class alternations (not nested (x+)+ style), so there's no catastrophic-backtracking exploit path even though the patterns grew longer.
  • Execution path confirmed safe. Traced input_match/pattern consumption to plugins/evals/skills/validate/scripts/validate-cases.py (js_regex, call_matches, grade_regex): patterns are compiled with Python's re.compile and applied via .search() against JSON-serialized tool-call logs — no eval, no shell, no dynamic code execution. These strings are authored by the PR, not attacker-influenced at eval time.
  • Instruction-surface lens: not applicable — these are eval graders (test scoring, not a live guardrail/allowlist a running skill enforces), and every change in scope strengthens detection; nothing is deleted, narrowed, or softened. (docs/conventions/instruction-exception-register/README.md was present and consulted for this determination.)

claude-lane-summary: lane=claude-security-review sha=c7acfae5b3ab18a101d0fbc11085af47566f90d6 run=38513337751

Comment thread plugins/architecture/evals/mc-two-deployables-stops/case.yaml Outdated
The no-components-shell-write grader's python -c / node -e branch matched
any mention of components.md, including read-only calls. Require the
write/dump/mode group in the same call, either order, as the ml and rd
sibling graders do, and add a read-only must-pass sample.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 3s —— View job


Security review complete

  • Gather context (incremental diff, custom instructions)
  • Invoke /review:security-review
  • Review the changed grader/sample files
  • Adversarially verify candidate findings
  • Post final results

Scope: incremental — only the 2 files this lane's instructions named, as diffed since c7acfae5b3ab18a101d0fbc11085af47566f90d6 (per .claude-lane/incremental.diff):

  • plugins/architecture/evals/mc-two-deployables-stops/case.yaml
  • plugins/architecture/evals/mc-two-deployables-stops/samples/no-components-shell-write.json

No security issues found.

This diff narrows the no-components-shell-write grader's python3/node alternative (added in this PR) by requiring a write/dump/mode-flag token (write, dump, 'w'/'w+', 'a'/'a+') to appear either before or after the components.md mention, and adds a matching read-only pass fixture. This is exactly the fix the earlier claude-review lane requested on mc-two-deployables-stops/case.yaml (the python3 -c "print(open(...).read())" read was previously matched as a forbidden write).

What I checked:

  • No secrets or credentials. The new sample command and why string are synthetic eval fixture content (a read of docs/architecture/components.md), not transcript or credential-shaped text.
  • No injection/execution surface. The pattern is consumed by validate-cases.py via Python's re.compile(...).search() against JSON-serialized tool-call text — no eval, no shell, no dynamic code execution, and the pattern is authored by this PR, not attacker-influenced at eval time.
  • No ReDoS. The added alternation ((?:write|dump|'[wa]\+?'|\"[wa]\+?\")(?:[^"\\]|\\.)*?components\.md and its mirrored ordering) uses the same bounded lazy-quantifier-over-negated-character-class structure already present and previously assessed safe; no nested unbounded quantifiers were introduced.
  • Instruction-surface lens: not applicable — this is an eval grader (test scoring for the outcome suite), not a live guardrail/allowlist a running skill enforces, and the change tightens the fix rather than removing or softening any check. (docs/conventions/instruction-exception-register/README.md is present and was consulted.)

One gap remains in this grader's coverage (e.g. shutil.copy/os.rename/'wb' mode writes via python3 -c still wouldn't trip no-components-shell-write, since they contain no write/dump/'w'/'a' token), but per /review:security-review's scope this is test-coverage of an eval fixture, not a security finding — it belongs with the claude-review lane, consistent with how the author has deferred this grader's other gaps to #7438.

claude-lane-summary: lane=claude-security-review sha=84668926ff18c12e05c3ec0a2f2f5bbe826849c8 run=38514210540

@claude

claude Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 3m 23s —— View job


Code review complete

  • Gather context (incremental diff .claude-lane/incremental.diff, custom instructions)
  • Review mc-two-deployables-stops/case.yaml grader change (line 34)
  • Review mc-two-deployables-stops/samples/no-components-shell-write.json fixture change (new pass sample, lines 22-33)
  • Adversarially verify candidate findings (manual char-by-char regex trace, since this lane's permission mode denies python3/node execution)
  • Post inline comment / summary

Scope: incremental — only these two files changed since this lane's last review of c7acfae5b3. This diff adds a write-action requirement (write/dump/quoted 'w'/'a' mode flag) to the python3/node alternative of no-components-shell-write, fixing the false-positive this lane flagged on that commit (a read-only python3 -c "print(open(...).read())" call was wrongly matching max: 0), plus a matching read-only pass fixture.

1 finding posted inline:

  1. mc-two-deployables-stops/case.yaml:34 — the fix correctly stops the false-positive, but narrows matching to literal write/dump/mode-flag tokens, so an interpreter write via an API whose name contains none of those (shutil.move, fs.copyFileSync, os.replace, etc.) into components.md now goes undetected — a real narrowing versus the pre-fix (over-broad) pattern, verified by manual trace against all existing pass/fail fixtures plus the two new bypass examples quoted in the comment.

What I verified and found sound:

  • Traced the updated regex against every sample in no-components-shell-write.json: the new read-only pass fixture no longer matches (fixes the prior finding), and all three existing fail fixtures (.write(, pathlib.write_text, fs.writeFileSync) still match via the write/mode-flag tokens in either ordering (verb-before-path and path-before-verb).
  • No ReDoS concern — the added alternation is unambiguous character-class/literal matching, not nested quantifiers on overlapping sets.
  • JSON fixture structurally valid, correctly placed under pass.

claude-lane-summary: lane=claude-review sha=84668926ff18c12e05c3ec0a2f2f5bbe826849c8 run=38514210471

Comment thread plugins/architecture/evals/mc-two-deployables-stops/case.yaml Outdated
…ders

The python -c / node -e write group in the mc, ml and rd graders missed
copy, move and rename APIs. Add those verbs to all three groups and add
must-fail samples for shutil.move and fs.copyFileSync.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Code review in progress

  • Gather context (incremental diff, custom instructions)
  • Invoke /review:code-review for the incremental scope
  • Review changed grader/sample files (mc, ml, rd)
  • Adversarially verify candidate findings
  • Post inline comments / summary

View job run

…the write graders

Drop the bare .replace( and link alternatives from the interpreter write
groups (str.replace and readlink are reads); keep os.replace and match
os/fs symlink and link calls. Add must-pass samples for both reads in the
ml and rd cases.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 47s —— View job


Security review complete

  • Gather context (incremental diff, custom instructions)
  • Invoke /review:security-review
  • Review the changed grader/sample files
  • Adversarially verify candidate findings
  • Post final results

Scope: incremental — only the 6 files this lane's instructions named, as diffed since 84668926 (per .claude-lane/incremental.diff, confirmed byte-identical to git diff 8466892..67d9ab0 -- <paths>):

  • plugins/architecture/evals/mc-two-deployables-stops/case.yaml + samples/no-components-shell-write.json
  • plugins/architecture/evals/ml-check-writes-nothing/case.yaml + samples/no-shell-write.json
  • plugins/architecture/evals/rd-no-convention-writes-nothing/case.yaml + samples/no-shell-write.json

No security issues found.

This diff extends three eval graders' python3/node write-detection alternative (added earlier in this PR to fix a false-negative on reads) with more write-indicating verbs — shutil.(move|copy*), os.(rename|replace), a generic .(rename|replace)( method call, copyFile(Sync)?, rename(Sync)?, cp(Sync)?(, and bare link — plus two matching must-fail fixtures per case (shutil.move, fs.copyFileSync). This directly closes the shutil.move/copyFileSync bypass flagged on mc-two-deployables-stops in the prior claude-review lane pass on 84668926.

Checked:

  • No secrets or credentials. All new content is regex alternatives and static JSON fixture strings (/tmp/x.md, docs/architecture/components.md, docs/architecture/landscape.md, docs/adr/0001-postgres.md); no credential-shaped or canary strings touched.
  • No ReDoS. The added alternatives (shutil\.(?:move|copy\w*), os\.(?:rename|replace), \.(?:rename|replace)\(, copyFile(?:Sync)?, rename(?:Sync)?, cp(?:Sync)?\(, link) are fixed literals or single bounded \w*/? quantifiers inside the existing alternation — no nested unbounded quantifiers over overlapping character classes, so no new catastrophic-backtracking path versus the pattern already assessed safe on 84668926.
  • Execution path unchanged. These strings are still consumed only by validate-cases.py's re.compile(...).search() against JSON-serialized tool-call logs — no shell, no eval, no dynamic code execution — and are authored by this PR, not attacker-influenced at eval time.
  • Instruction-surface lens: not applicable — this is an eval grader (test scoring for the outcome suite), not a live guardrail a running skill enforces, and the change is strictly additive (more write verbs detected, nothing removed, narrowed, or softened). (docs/conventions/instruction-exception-register/README.md is present in this checkout.)

Noted, not reported as a finding (consistent with this PR's own triage of grader-completeness gaps to #7438): the new verb list still omits some interpreter-driven write/create paths outside its own scope (e.g. pathlib.Path(...).touch(), os.mknod, fs.writeSync without a literal write substring before the flag check). These are test-coverage gaps in a non-production eval fixture, not an exploitable security surface — they belong with claude-review/#7438, the same place the author has been routing every prior grader-coverage gap in this PR.

claude-lane-summary: lane=claude-security-review sha=67d9ab0880d604809caada6e38c13a26bce8ec6e run=38514687356

@claude

claude Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 44s —— View job


Security review complete

  • Gather context (incremental diff, custom instructions)
  • Invoke /review:security-review
  • Review the changed grader/sample files
  • Adversarially verify candidate findings
  • Post final results

Scope: incremental — only the 5 files this lane's instructions named, as diffed since 67d9ab0880 (per .claude-lane/incremental.diff):

  • plugins/architecture/evals/mc-two-deployables-stops/case.yaml
  • plugins/architecture/evals/ml-check-writes-nothing/case.yaml
  • plugins/architecture/evals/ml-check-writes-nothing/samples/no-shell-write.json
  • plugins/architecture/evals/rd-no-convention-writes-nothing/case.yaml
  • plugins/architecture/evals/rd-no-convention-writes-nothing/samples/no-shell-write.json

No security issues found.

This diff narrows the python3/node interpreter-write alternative shared by mc no-components-shell-write, ml no-shell-write, and rd no-shell-write: the generic .(?:rename|replace)\( token is narrowed to .rename\( only, and the bare, unanchored link substring is narrowed to (?:os|fs)\.(?:sym)?link(?:Sync)?. Matching new must-pass fixtures (python3 -c "print('a-b'.replace('-','_'))" and python3 -c "import os; print(os.readlink('docs'))") are added to ml and rd's no-shell-write.json. This is a false-positive fix: the prior unanchored link token matched the "link" substring inside unrelated calls like os.readlink(...), and bare .replace( matched any str.replace() call, so read-only interpreter calls were previously (wrongly) scored as forbidden writes.

What I checked:

  • Scope classification. These are eval graders — tool_used/regex scoring consumed by validate-cases.py via Python's re.compile(...).search() against JSON-serialized tool-call logs — not a live guardrail/allowlist a running skill enforces during real usage. Per the instruction exception register (read, present in this checkout), the protected classes (irreversible-action, secret-handling, data-integrity, external-publication, legal-compliance, agent-authority) govern standing instructions that gate live agent actions. A test-scoring regex that only runs after the fact inside a sandboxed eval doesn't gate anything at the moment of action, so the instruction-surface lens doesn't apply here — consistent with this lane's own determination earlier in this PR's thread on the sibling mc/ml grader edits.
  • No new secrets or credential-shaped strings in either changed sample file — the new fixtures are trivial string literals ('a-b', 'docs').
  • No injection or ReDoS surface. The narrowing only removes/tightens literal alternatives in an existing alternation; no new unbounded or nested quantifiers were introduced, and the regex is still authored content (not attacker-influenced) consumed by re.search, not eval or a shell.
  • Residual coverage gap (not a security finding, noted for completeness): narrowing bare link to (?:os|fs)\.(?:sym)?link(?:Sync)? also drops the (unintentional, substring-based) match the old pattern had on os.unlink(...) — a destructive delete of the target file that arguably should still trip max: 0. This is grader test-coverage, the same category of gap already tracked under test(architecture): tighten eval graders the #7437 ready review found lenient #7438 for this grader family, not a live-guardrail or exploit-path issue in scope for this lane.

claude-lane-summary: lane=claude-security-review sha=6f04a325e515791d29e410a79654b4b67c0b930c run=38514732804

@claude

claude Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 3m 8s —— View job


Code review complete

  • Gather context (incremental diff .claude-lane/incremental.diff, custom instructions)
  • Review mc-two-deployables-stops grader/sample changes
  • Review ml-check-writes-nothing grader/sample changes
  • Review rd-no-convention-writes-nothing grader/sample changes
  • Adversarially verify candidate findings (manual regex trace; python3/node execution is denied under this lane's permission mode)
  • Post inline comments / summary

Scope: incremental — only the 6 files this lane's instructions named, as diffed since 8466892. This diff extends the interpreter-write detection added in the previous round (shutil.move/copy*, os.rename/replace, .rename(, copyFile(Sync)?, rename(Sync)?, cp(Sync)?(, (os|fs).(sym)?link(Sync)?) across mc's no-components-shell-write, ml's no-shell-write, and rd's no-shell-write, each with matching shutil.move/copyFileSync must-fail samples and the str.replace/os.readlink must-pass samples that prove the group doesn't over-match.

3 findings posted inline, one per grader — the same remaining gap in each:

pathlib's Path.replace() (a rename/overwrite, same semantics as os.replace()) isn't covered by any alternative in the new group — python3 -c "from pathlib import Path; Path('/tmp/x.md').replace('docs/architecture/components.md')" matches neither os\.(?:rename|replace) (needs the os. prefix) nor \.rename\( (needs rename, not replace), so each max: 0 grader still passes despite the write. Adding a bare \.replace\( alternative isn't a safe fix on its own, since it would flip the newly-added str.replace must-pass fixture ('a-b'.replace('-','_')) into a false match — nothing in the pattern currently distinguishes a Path receiver from a plain string.

What I verified and found sound:

  • The shutil.move/copyFileSync cases this group was added for (per the prior round's finding) now correctly match in all three files; traced the full composed regex against both new must-fail samples in each samples/*.json.
  • The two new must-pass samples ('a-b'.replace('-','_') and os.readlink('docs')) correctly stay unmatched: readlink doesn't trip (?:os|fs)\.(?:sym)?link(?:Sync)? because the group requires link/symlink immediately after the os./fs. dot, and readlink has read in between.
  • No new ReDoS surface — all additions are bounded alternations, no nested unbounded quantifiers.
  • JSON fixture additions in all three samples/*.json are well-formed and correctly bucketed under pass/fail.

Note on verification method: this lane's permission mode denies python3/node execution, so the above is a manual, character-by-character trace against Python's re semantics (which validate-cases.py itself uses for grading) rather than an executed test.

claude-lane-summary: lane=claude-review sha=6f04a325e515791d29e410a79654b4b67c0b930c run=38514732821

Comment thread plugins/architecture/evals/mc-two-deployables-stops/case.yaml Outdated
Comment thread plugins/architecture/evals/ml-check-writes-nothing/case.yaml Outdated
Comment thread plugins/architecture/evals/rd-no-convention-writes-nothing/case.yaml Outdated
…raders

Match .replace( only when its first argument is a quoted path containing
a slash or ending in a doc extension, so Path.replace is caught while
str.replace('-', '_') stays a read. Add a must-fail sample per case.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 17s —— View job


Security review complete

  • Gather context (incremental diff, custom instructions)
  • Invoke /review:security-review
  • Review the changed grader/sample files
  • Adversarially verify candidate findings
  • Post final results

Scope: incremental — only the 6 files this lane's instructions named, as diffed since 6f04a325e515791d29e410a79654b4b67c0b930c (per .claude-lane/incremental.diff):

  • plugins/architecture/evals/mc-two-deployables-stops/case.yaml + samples/no-components-shell-write.json
  • plugins/architecture/evals/ml-check-writes-nothing/case.yaml + samples/no-shell-write.json
  • plugins/architecture/evals/rd-no-convention-writes-nothing/case.yaml + samples/no-shell-write.json

No security issues found.

This diff adds a \.replace\( alternative to the shared interpreter-write detection group in all three graders' python3/node alternation, gated by a lookahead requiring the argument to look like a path (contains / or a .md/.markdown/.yaml/.yml extension inside the quoted string). It closes the pathlib.Path(...).replace(...) gap this lane and claude-review both flagged on 6f04a325, without reintroducing a match on the existing str.replace('-','_') must-pass fixture (traced: that argument has no / or recognized extension inside its quotes, so the lookahead fails and the alternative doesn't fire). Each of the three files gets one matching must-fail fixture (mc/samples/no-components-shell-write.json#L194-205, ml/samples/no-shell-write.json#L225-236, rd/samples/no-shell-write.json#L222-233), each placed in the fail array (verified by line offset against the "fail": key in each file).

What I checked:

  • No secrets or credentials. The three new fixture commands are synthetic python3 -c "from pathlib import Path; Path('/tmp/x.md').replace(...)" strings naming fake repo paths (docs/architecture/components.md, docs/architecture/landscape.md, docs/adr/0001-postgres.md); no credential-shaped or canary content.
  • No ReDoS. The new lookahead ((?=\s*(?:'|\\")[^'\\"]*(?:/|\.(?:md|markdown|ya?ml))[^'\\"]*(?:'|\\"))) has the same A*literalA*literal shape already used throughout this grader family — the negated character class excludes the delimiter characters it scans toward, so matching is a bounded linear/quadratic scan, not nested unbounded quantifiers. No new catastrophic-backtracking surface versus the pattern already assessed safe on prior commits in this PR.
  • Execution path unchanged. Still consumed only by validate-cases.py's re.compile(...).search() against JSON-serialized tool-call logs — no shell, no eval, no dynamic code execution — and the pattern is authored by this PR, not attacker-influenced at eval time.
  • Instruction-surface lens: not applicable, consistent with this lane's determination on the identically-shaped diffs earlier in this PR — these are eval graders (test scoring after the fact), not a live guardrail a running skill enforces, and the change is strictly additive (closes a known-gap, narrows nothing).

Not reported as a finding (grader-correctness, not security — belongs with claude-review/#7438 per this PR's own triage pattern): the lookahead's path heuristic (/ or a known extension inside the quotes) means Path('/tmp/x').replace('docs/architecture/components') — a path with no extension and no further / after the directory — would still slip past if the destination had no slash beyond what's already matched... but note the three committed must-fail fixtures all use paths with a .md extension, which this lookahead does catch. This is a residual coverage edge, not an exploitable surface.

claude-lane-summary: lane=claude-security-review sha=07598a6960a0bc4e06bcb69d893a62a3289ca4d5 run=38515628739

@claude

claude Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 6m 28s —— View job


Code review complete

  • Gather context (incremental diff .claude-lane/incremental.diff, custom instructions)
  • Review mc-two-deployables-stops grader/sample changes
  • Review ml-check-writes-nothing grader/sample changes
  • Review rd-no-convention-writes-nothing grader/sample changes
  • Adversarially verify candidate findings (manual regex trace; python3 execution is denied under this lane's permission mode)
  • Post inline comments / summary

Scope: incremental — only the 6 files this lane's instructions named, as diffed since 6f04a325. This diff adds a .replace( alternative (with a lookahead requiring a quoted, path-like argument) to the shared interpreter-write detection group in mc's no-components-shell-write, ml's no-shell-write, and rd's no-shell-write, closing the Path.replace() bypass flagged on the prior commit, plus matching must-fail "pathlib replace" fixtures in all three samples/*.json.

3 findings posted inline, one per file — the same residual gap in each:

The new .replace( lookahead requires the character right after .replace( to be an opening quote, so it catches the literal-string form (Path(src).replace('docs/.../components.md'), the fixture just added) but not the equally idiomatic pathlib form that passes another Path object as the argument, e.g. Path('/tmp/x.md').replace(Path('docs/architecture/components.md')). There, .replace( is immediately followed by Path(, not a quote, so the lookahead fails, and no other verb token in the group (write, dump, shutil.*, os.rename/os.replace, .rename(, copyFile*, rename*, cp(*, (os|fs).(sym)?link*, quoted 'w'/'a') matches either — each max: 0 grader still passes despite the write.

What I verified and found sound:

  • The literal-string Path(...).replace('...') bypass this round fixes now correctly matches in all three files (traced each new "pathlib replace" fail fixture through the full composed regex).
  • The must-pass 'a-b'.replace('-','_') fixture correctly stays unmatched: the lookahead's [^'\"]* can't cross the closing quote of '-' to find a / or extension, so it never reaches the .replace( alternative.
  • No new false-positive risk from the lookahead itself for ordinary string .replace() calls on non-path-like literals.
  • The lookahead regex is syntactically balanced (5 open/5 close non-capturing-group parens, properly nested).

Note on verification method: this lane's permission mode denies python3 execution (confirmed: even python3 -I -c "print(1)" requires approval), so the above is a manual, character-by-character trace against Python's re semantics (which validate-cases.py itself uses for grading) rather than an executed test, consistent with this lane's prior passes on this same PR.

claude-lane-summary: lane=claude-review sha=07598a6960a0bc4e06bcb69d893a62a3289ca4d5 run=38515628694

Comment thread plugins/architecture/evals/ml-check-writes-nothing/case.yaml Outdated
Comment thread plugins/architecture/evals/rd-no-convention-writes-nothing/case.yaml Outdated
Comment thread plugins/architecture/evals/mc-two-deployables-stops/case.yaml Outdated
Path.replace takes one argument and str.replace takes two or more, so
match .replace( followed by one argument with no top-level comma instead
of guessing the argument's form. Add must-fail samples for a Path
argument and a variable target, and must-pass samples for two-argument
replace calls.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 47s —— View job


Security review complete

  • Gather context (incremental diff, custom instructions)
  • Invoke /review:security-review
  • Review the changed grader/sample files
  • Adversarially verify candidate findings
  • Post final results

Scope: incremental — only the 6 files this lane's instructions named, as diffed since 07598a6960 (per .claude-lane/incremental.diff):

  • plugins/architecture/evals/mc-two-deployables-stops/case.yaml + samples/no-components-shell-write.json
  • plugins/architecture/evals/ml-check-writes-nothing/case.yaml + samples/no-shell-write.json
  • plugins/architecture/evals/rd-no-convention-writes-nothing/case.yaml + samples/no-shell-write.json

No security issues found.

This diff replaces the .replace( quoted-literal lookahead (added last round) with a single-argument-call lookahead — (?=\s*[^,()]*(?:\([^()]*\)[^,()]*)?\)) — in all three graders' shared python3/node interpreter-write alternation. It closes the Path(...).replace(Path(...)) / .replace(variable) gap flagged on the prior commit (07598a6), since pathlib.Path.replace() always takes exactly one argument while str.replace() always takes two — matching only a single-argument .replace( call is a sound way to distinguish the two without needing to recognize a Path receiver. Each of the three files adds two matching "pathlib replace" must-fail fixtures (Path(...) argument, variable-target argument) and two "two-argument replace" must-pass fixtures (plain string replace, and a Path(...).name used as the first of two arguments) to their samples/*.json.

What I checked:

  • No secrets or credentials. All six new fixture commands are synthetic python3 -c strings using fake repo paths (docs/architecture/components.md, docs/architecture/landscape.md, docs/adr/0001-postgres.md) and placeholder values ('x y', 'x', 'z', 'q'); no credential-shaped or canary content.
  • No ReDoS. The new lookahead's optional group (?:\([^()]*\)[^,()]*)? is a single ? (0-or-1) occurrence, not a repeated/nested quantifier — it can't recurse across multiple paren levels, so there's no exponential-backtracking surface. The two [^,()]* scans are bounded, mutually-exclusive-character-class scans (same shape already assessed safe on prior commits in this PR), giving at most polynomial backtracking on these short, non-adversarial eval-fixture strings.
  • Execution path unchanged. Still consumed only by validate-cases.py's re.compile(...).search() against JSON-serialized tool-call logs (traced manually; this lane's permission mode denies python3 execution, consistent with its prior passes on this same PR) — no shell, no eval, no dynamic code execution, and the pattern is authored by this PR, not attacker-influenced at eval time.
  • Instruction-surface lens: not applicable, consistent with this lane's determination on every identically-shaped diff earlier in this PR's thread — these are eval graders (test scoring after the fact), not a live guardrail a running skill enforces, and the change is strictly additive/corrective (closes a known gap, narrows nothing that was previously relied on for a real guarantee).

Not a security finding (grader-correctness, belongs with claude-review/#7438 per this PR's own triage pattern): manually traced the new lookahead and could not find a remaining single-argument-call bypass for these three fixtures, but did not have python3/node execution available to confirm against a live regex engine — any residual edge case (e.g. a single-argument call using a different nested-call shape) would be a test-coverage gap in a non-production fixture, not an exploitable surface.

claude-lane-summary: lane=claude-security-review sha=bc2ab79888654ca8360bab1ed367f0c79f639697 run=38516387894

@claude

claude Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 3m 50s —— View job


Code review complete

  • Gather context (incremental diff .claude-lane/incremental.diff, custom instructions)
  • Review mc-two-deployables-stops/case.yaml + samples/no-components-shell-write.json
  • Review ml-check-writes-nothing/case.yaml + samples/no-shell-write.json
  • Review rd-no-convention-writes-nothing/case.yaml + samples/no-shell-write.json
  • Adversarially verify candidate findings (manual regex trace; python3 execution is denied under this lane's permission mode)
  • Post inline comments / summary

Scope: incremental — only the 6 files named in this lane's instructions, as diffed since 07598a6960. This diff replaces the quote-only .replace( lookahead with (?=\s*[^,()]*(?:\([^()]*\)[^,()]*)?\)), which recognizes a single-argument .replace() call regardless of whether the argument is quoted, a bare variable, or wrapped in one function call (Path(...)), closing the two bypasses flagged on the prior commit (Path(Path(...)) and a variable target). Matching "pathlib replace with Path argument" / "pathlib replace with variable target" must-fail fixtures are added to all three files, plus "two-argument replace" must-pass fixtures proving the broader lookahead doesn't reintroduce a false positive on ordinary str.replace(old, new) calls.

3 findings posted inline, one per file — the same residual gap in each:

The new lookahead tolerates at most one level of parenthesized nesting inside the .replace() argument. A second level — e.g. Path('/tmp/x.md').replace(Path(str(target))), wrapping the target in str(), a common idiom when a Path is passed where a plain string is expected — still slips past: the optional nested-group attempt can't cross str('s own parens, so it never finds the closing ) it needs, and no other verb token (write, dump, shutil.*, os.rename/replace, .rename(, copyFile*, rename*, cp(*, (os|fs).(sym)?link*, quoted 'w'/'a') appears in the command either. max: 0 still passes despite the write.

What I verified and found sound:

  • Traced both new must-fail fixtures (Path(Path(...)) and the variable-target case) through the full composed regex in all three files: both now correctly match.
  • Traced both new "two-argument replace" must-pass fixtures (s.replace(k, v) and s.replace(Path('x').name, 'q')): both correctly stay unmatched — the top-level comma in each blocks the lookahead, and no other token fires.
  • No regression on the pre-existing single-quoted-literal fail fixture (Path(...).replace('docs/.../components.md')) — still matches.
  • JSON fixture additions in all three samples/*.json are well-formed and correctly bucketed under pass/fail.

Note on verification method: this lane's permission mode denies python3 execution (confirmed: even a trivial python3 -I -c "..." requires approval), so the above is a manual, character-by-character trace against Python's re semantics (which validate-cases.py itself uses for grading) rather than an executed test, consistent with this lane's prior passes on this same PR.

claude-lane-summary: lane=claude-review sha=bc2ab79888654ca8360bab1ed367f0c79f639697 run=38516387604

Comment thread plugins/architecture/evals/mc-two-deployables-stops/case.yaml
Comment thread plugins/architecture/evals/ml-check-writes-nothing/case.yaml
@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 14, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 14, 2026
Lane-Activity: pr-unblock/resolve-conflicts
@claude

claude Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @melodic-automation-lanes[bot]'s task in 1m 15s —— View job


Code review complete

  • Gather context (diff since this lane's last review at bc2ab798, custom instructions)
  • Invoke /review:code-review
  • Check what changed since the last reviewed commit
  • Adversarially verify candidate findings
  • Post inline comments / summary

Scope: incremental — this lane's last pass reviewed through bc2ab798 (3 findings on the .replace() arity fix, which the author declined to extend further and deferred to #7438, consistent with every earlier interpreter-write round in this thread).

Skip gate: no new review surface. git diff bc2ab798..74a1c24f -- plugins/architecture .changes .gitleaksignore shows the only changes since that review are from 74a1c24f ("Merge main into feat/architecture-concise-skills"), and none of them touch this PR's own content:

  • .changes/...feat-harness-ops-concise-skills-...md removed / ...feat-session-flow-concise-skills-...md added — changelog fragments from the unrelated #7442 (session-flow/discipline/harness-ops/work-items) merge, not this PR's architecture fragment.
  • .gitleaksignore +4 lines — all four are plugins/session-flow/evals/fh-confirms-before-resume/... entries from that same unrelated merge, not plugins/architecture.

git diff origin/main...HEAD -- plugins/architecture .changes .gitleaksignore confirms this PR's actual scope is unchanged from what this lane already reviewed through bc2ab798.

No new findings. Per the skip gate ("this head already has a successful review from this lane that still applies"), there is nothing new to review: every finding raised earlier in this thread (mc/ml/rd interpreter-write grader gaps, mp's negation-guard asymmetry) was fixed, and the remaining residual gap (arbitrarily-nested .replace() call arguments) was explicitly declined and deferred to #7438 by the author with sound reasoning (matching nested call arguments to arbitrary depth isn't expressible as a regular-language regex).

0 findings to report.

claude-lane-summary: lane=claude-review sha=74a1c24f7c6a3988e820116940601fb6f7a5e027 run=38576304076

@claude

claude Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @melodic-automation-lanes[bot]'s task in 1m 29s —— View job


Security review complete

  • Gather context (repo instructions, PR diff at head 74a1c24)
  • Invoke /review:security-review and follow its instructions
  • Review changed eval fixtures/scaffolds/graders for secret-handling and injection risk
  • Adversarially verify candidate findings
  • Post final results

Scope: full PR — git diff origin/main...HEAD (merge-base ea0f9aa5, which is origin/main's current tip) touches only plugins/architecture/** plus .gitleaksignore and .changes/architecture/..., 111 files / +4485 / −224.

No security issues found — this head needs no new review (skip-gate condition 3).

This trigger fired on a "synchronize" event, but the only commit added since this lane's last full pass (bc2ab79, posted earlier on this PR) is 74a1c24, a merge of origin/main into this branch. I checked git diff bc2ab79..74a1c24 -- plugins/architecture .gitleaksignore .changes/architecture: the only line-level change is four new .gitleaksignore entries for plugins/session-flow/evals/fh-confirms-before-resume/..., and git show ea0f9aa5:.gitleaksignore confirms those four lines already exist verbatim on origin/main — they arrived here only because origin/main is now exactly this merge's first parent (git merge-base origin/main HEAD = ea0f9aa5 = origin/main's tip), not because this PR authored them. No file under plugins/architecture/ (the PR's actual content) differs between bc2ab79 and 74a1c24.

Every file this PR actually owns — all 11 SKILL.md diffs, the ten new eval cases and their scaffolds/graders/samples, and the eleven probes/*.json trigger sets — was already reviewed incrementally by this lane across the thread on 9c76a3d7 → 020b2e0d → c7acfae5 → 8466892 → 67d9ab08 → 6f04a325 → 07598a69 → bc2ab798, with no issues found at any step (canary-fixture secret handling fixed and verified byte-identical, grader regex changes checked for ReDoS and execution-path safety, each narrowing/widening of the eval grader regexes assessed). Since none of that content changed under this head, that prior clearance still applies and there is nothing new to hunt for.

claude-lane-summary: lane=claude-security-review sha=74a1c24f7c6a3988e820116940601fb6f7a5e027 run=38576304358

@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 14, 2026
Merged via the queue into main with commit cfa909f Oct 14, 2026
25 checks passed
@kyle-sexton
kyle-sexton deleted the feat/architecture-concise-skills branch October 14, 2026 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(architecture): tighten eval graders the #7437 ready review found lenient

1 participant