Skip to content

Release awf 0.0.2: cr-receive 0.0.1, cr-request 0.0.2, auto review without F2 - #2

Merged
ihororlovskyi merged 8 commits into
mainfrom
261018-dev-session
Oct 18, 2026
Merged

ihororlovskyi merged 8 commits into
mainfrom
261018-dev-session

Conversation

@ihororlovskyi

@ihororlovskyi ihororlovskyi commented Oct 18, 2026 •

Copy link
Copy Markdown
Member

Summary

Release awf v0.0.2: new cr-receive 0.0.1, cr-request 0.0.2, and the yngwald auto review moved to cr-request with the F2 file-write risk closed.

  • cr-receive 0.0.1 (new): receives a code review and resolves it with evidence. Intake follows the cr-request result contract (axis verdicts, findings with confidence, set-aside list, open questions, merge readiness) and normalizes any other source to the same shape. Findings are anchored to the reviewed target, recorded one per item, verified as claim and remedy separately, and decided as ACCEPT, PARTIAL, REJECT, DEFER, or ESCALATE. Fixes go in dependency order with per-finding evidence, re-review is scoped and bounded, replies state facts.
  • cr-request 0.0.2: hands its result to cr-receive, and accepts a prepared target (identity, log, diff, and base copies captured by the caller) for runs without Git access.
  • claude-review.yml: the auto review runs cr-request installed from the v0.0.1 tag commit (39626fc), never from the PR head. F2 is closed: a Prepare review target step recreates .auto-review/ (so PR symlinks are not followed) and writes the target files there, and the agent loses git diff, git log, and git show, which accept --output=<path>. The allowlist keeps read-only git merge-base, rev-parse, status, and ls-files.
  • Both workflows pin actions/checkout to the v5.1.0 commit; ci.yml reads metadata.version only from the metadata block.
  • Manifests, marketplaces, and README at 0.0.2 with both skills; the README License section is removed. AGENTS.md notes the sentimony/skills marketplace ref bump after a release tag.

Checks

  • All three CI steps pass locally; git diff --check is clean.
  • The Prepare review target step, run locally with .auto-review/target committed as a symlink to an outside file, leaves that file intact and produces the target files.
  • awf@agentic-workflow installs in temporary Claude Code and Codex configs with both skills under awf/0.0.2.
  • Cross-review: Antigravity round 1 PASS; Codex round 1 F1 (Important, symlink write) fixed, round 2 PASS.

After merge

Tag v0.0.2, then bump CR_REQUEST_REF to the tag commit and the sentimony/skills marketplace awf ref to v0.0.2.

- claude-review.yml: the auto review runs cr-request from the v0.0.1 tag
  commit instead of review-request from sentimony/skills; the prompt,
  allowed read-only git commands, and the skill-line checks follow it.
- claude-review.yml: document the accepted --output risk of the git
  prefixes in the allowlist (auto review finding F2).
- Both workflows pin actions/checkout to the v5.1.0 commit.
- ci.yml: metadata.version is read only from the metadata block.
@yngwald

yngwald commented Oct 18, 2026

Copy link
Copy Markdown
Collaborator

Automated review - PR #2

Target reviewed: committed range 39626fcce7cadd6a77b1cf5ebc6b0ce922f37960...b5ac9a3850091c9e062a26c13f6e968f7c69b697 (merge base = base SHA 39626fc, one commit b5ac9a3). Files: .github/workflows/ci.yml (+6/-2), .github/workflows/claude-review.yml (+27/-22). Working tree clean, nothing uncommitted or untracked in scope.

Review mode: independent reviewer, single pass (no earlier reviews or comments on this PR, so this is not a re-review). No specialist reviewers were dispatched; the change is CI-only, so the risk is medium (CI supply chain and the permissions of an automated agent).

Requirements: the PR title and description, plus AGENTS.md.

Coverage: I read the whole diff and the full post-change claude-review.yml, plus lines 18-132 of ci.yml. I also read the pinned skill at 39626fc: plugins/awf/skills/cr-request/SKILL.md, references/target.md and references/brief.md. The tag v0.0.1 points at 39626fc, which I checked locally.

Verdicts

Axis Verdict
Requirements/spec compliance PASS
Scope compliance PASS
Code/engineering quality FINDINGS
Risk/domain concerns UNASSESSED - see the review gaps
  • Requirements: PASS. Every bullet in the description is implemented:

    • the cr-request skill is installed from the v0.0.1 commit, not from the PR head;
    • the prompt and output contract now ask for the cr-request result (Intent, System and Craft verdicts, confidence, set-aside list, open questions, merge readiness), and that matches references/brief.md at 39626fc;
    • the read-only git rev-parse, git status and git ls-files were added to the allowlist;
    • the post and verify steps now match Reviewed with: cr-request skill;
    • a comment records the accepted F2 risk;
    • both workflows pin checkout by SHA;
    • metadata.version is now read only from the metadata block.

    Under AGENTS.md, no plugin or skill content changes, so no version or CHANGELOG bump is needed.

  • Scope: PASS. Every hunk maps to one of the bullets in the description. There are no unrelated edits.

  • Engineering: FINDINGS. There is one Minor finding (F1) and one suggestion.

  • Risk/domain: UNASSESSED. I found no defects, but I could not confirm the checkout SHA-to-tag mapping, and no live run of the changed workflow exists yet (see the gaps).

Findings

F1 - Minor - .github/workflows/claude-review.yml:168

  • Issue: The allowlist still leaves out git branch --show-current, even though cr-request's references/target.md (at 39626fc) lists it with the read-only commands it uses to pin the target. The working-tree fingerprint commands (git hash-object, and a pipe) are also not allowed. Those only matter for working-tree targets, so they are irrelevant here.
  • Impact: The PR description says the allowlist adds what "cr-request uses to pin the target", but one of those commands will be denied. In CI the checkout is a detached HEAD, so the command would print nothing anyway. The agent has to absorb a denied tool call and fill in "Branch or worktree" in some other way. This spends turns and leaves room for a missing or inconsistent identity field. It does not break correctness.
  • Evidence: target.md "Read-only commands" lists git rev-parse --show-toplevel, git branch --show-current, git rev-parse HEAD and git status --porcelain=v1 --untracked-files=all. The --allowedTools value at line 168 has no Bash(git branch:*).
  • Direction: Either allow Bash(git branch --show-current) exactly, or say in the prompt that the target is a detached checkout and branch discovery should be skipped. A broad git branch:* would allow creating or deleting branches, so avoid it.

Suggestions (optional, not blocking)

  • ci.yml:41 and ci.yml:128 repeat the same metadata: block regex, and the two version patterns differ: semver-strict at line 42, [^"]+ at line 129. The difference is defensible, because step 1 already enforces semver. A short comment saying that step 2 relies on step 1 for the format would stop the two copies drifting apart later.
  • Reasoning about the new block regex ^metadata:[ \t]*\n((?:[ \t]+.*\n?)*): the block ends at the first blank line or the first line at column 0. That means a blank line or a column-0 comment inside metadata: would hide a later version. It fails closed with a clear error, so this is acceptable, but the convention is worth knowing.

Set aside

  • The --output=<path> write risk of the git diff and git log prefixes. It is already recorded as accepted risk at claude-review.yml:162-164. The mitigations it lists match the file: the gate rejects forks (line 68), the job has read-only contents and pull-requests permissions, and the post step reads only .auto-review/comment.md. I did not reopen it.
  • A new skill name that the posted comments do not yet carry. Earlier yngwald comments carry review-request, and the verify step only checks comments with this head SHA's marker, so changing the expected skill line breaks nothing for old heads.

Review gaps

  • Tests: none were run in this job, including the CI Python scripts and a dry run of the workflow. The CI checks (skill frontmatter, manifests and marketplaces, README versions, hidden Unicode) are reported green on HEAD SHA; that is a signal, not proof.
  • Regex behavior: I could not run the new metadata regex locally in this sandbox. Its behavior above comes from reading the code, not from execution.
  • Checkout pin: I could not confirm that actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 is the v5.1.0 tag, because the GitHub API was not reachable from this job. Please confirm the mapping.
  • No live run of the change yet: this PR is reviewed by the workflow on main, which still uses review-request. So nothing has yet run the cr-request install step, the new prompt, or the new allowlist end to end. The first real signal is the next PR after merge, as the description notes.
  • Requirements beyond the PR: nothing exists beyond the PR description and AGENTS.md, so there is no separate spec or acceptance criteria.

Reviewed with: review-request skill

Bump awf to 0.0.2, list both skills in manifests and README, and drop the
README License section.
The workflow captures identity, log, diff, and base copies before the agent
runs, and the agent loses git diff, log, and show. cr-request 0.0.2 accepts
a prepared target; cr-receive gets its own assessment state names.
@ihororlovskyi ihororlovskyi changed the title Review with cr-request, pin checkout, and tighten the version check Release awf 0.0.2: cr-receive 0.0.1, cr-request 0.0.2, auto review without F2 Oct 18, 2026
@yngwald

yngwald commented Oct 18, 2026

Copy link
Copy Markdown
Collaborator

Automated review - PR #2 (fresh review pass)

Target reviewed: committed range 39626fcce7cadd6a77b1cf5ebc6b0ce922f37960...69027063407a6a0cca636cc0df5545653083482e. The merge base equals the base SHA 39626fc. There are five commits (b5ac9a3, 30125b9, 1e49986, c21942f, 6902706) and 15 files (+377/-49). The working tree is clean, with nothing uncommitted or untracked in scope.

Review mode: fresh review pass. The same reviewer (yngwald) already reviewed the older head b5ac9a3. That comment was read as context only, and its PASS is not reused for this head. No specialist reviewer was dispatched.

Requirements: the PR title and description (gh pr view 2) and AGENTS.md. No other spec exists.

Risk: medium. The change touches the permissions and file writes of an automated CI agent, and it adds a new skill with instruction content.

Coverage: I read the whole diff:

  • the workflows ci.yml and claude-review.yml;
  • AGENTS.md, README.md, both plugin manifests, and .claude-plugin/marketplace.json;
  • the cr-request SKILL.md, CHANGELOG.md, references/brief.md and references/target.md;
  • the new cr-receive SKILL.md, references/records.md, CHANGELOG.md and LICENSE.

I also checked .agents/plugins/marketplace.json, which this range does not change and which has no version or description to update, and that the root LICENSE is still tracked.

Verdicts

Axis Verdict
Requirements/spec compliance PASS
Scope compliance PASS
Code/engineering quality FINDINGS
Risk/domain concerns FINDINGS
  • Requirements: PASS. Each bullet in the description is implemented.
    • cr-receive 0.0.1 is added. Its frontmatter name equals the directory, it has metadata.version 0.0.1, a matching CHANGELOG entry and the MIT LICENSE (Ihor Orlovskyi), and it is listed in the Claude Code manifest skills.
    • cr-request is at 0.0.2 with a matching CHANGELOG entry, the hand-off to cr-receive, and the "Prepared target" section.
    • Both manifests are at 0.0.2.
    • The README shows the repository version (v0.0.2), the plugin version (0.0.2) and both skill versions.
    • AGENTS.md has the line about bumping the marketplace ref.
    • The auto review installs cr-request from 39626fc, and the Prepare review target step is added.
    • git diff, git log and git show are dropped from the allowlist, and rev-parse, status and ls-files are added.
    • Checkout is pinned by SHA in both workflows, and ci.yml reads metadata.version only from the metadata block.
  • Scope: PASS. Every hunk maps to a bullet in the description. The removed README License section is stated in the description, and the root LICENSE file still exists.
  • Engineering: FINDINGS. Two Minor findings: F2 and F3.
  • Risk/domain: FINDINGS. One Minor finding: F1.

The F2 file-write risk itself looks closed:

  • No allowed Bash prefix that is left (git merge-base, git rev-parse, git status, git ls-files, gh pr view, gh pr diff, gh pr checks) can write a file, as far as I know their options.
  • rm -rf .auto-review removes a PR-shipped symlink itself, not what it points to.
  • Every later write goes into the freshly created directory.

Findings

F1 - Minor - .github/workflows/claude-review.yml:117

  • Issue: CR_REQUEST_REF pins cr-request at v0.0.1 (39626fc). That version has no "Prepared target" section. Its references/target.md tells the agent to run git log and git diff to pin the target, and its references/brief.md tells the reviewer to read other revisions with git show <rev>:<path>. This PR removes all three commands from the allowlist (line 199).
  • Impact: From merge until CR_REQUEST_REF is bumped, each auto review runs a skill whose instructions conflict with its tool permissions. Only the workflow prompt tells the agent to use .auto-review/target/ instead. The likely result is denied tool calls, wasted turns within --max-turns 60, and possibly a reviewer subagent that never got the "read the prepared files" instruction. Nothing in the repository enforces the bump. It exists only in the PR description's "After merge" list.
  • Evidence: git show 39626fc:plugins/awf/skills/cr-request/references/target.md has no "Prepared target" section. The 0.0.2 changes to target.md:54-60 and brief.md:26-27 are what make the skill support this mode. The description says: "Tag v0.0.2, then bump CR_REQUEST_REF to the tag commit".
  • Direction: Bump the ref right after tagging, as planned. You could also add a # TODO: bump to v0.0.2 after tagging comment next to the ref, or make the prompt also tell any dispatched reviewer to read only .auto-review/target/.

F2 - Minor - .github/workflows/claude-review.yml:138

  • Issue: The prepared identity.txt does not follow the identity record in cr-request references/target.md:7-16. It writes Repository: <owner/name> instead of Repository root, and it has no Branch or worktree, Included paths, or Excluded paths and reason.
  • Impact: cr-request 0.0.2 says to "use it as given", so a reviewer copies an incomplete identity into the result. cr-receive step 2 (records.md:16-18) compares HEAD, the base, the included paths and the fingerprint to decide whether a finding is stale, and "included paths" is missing. This is low impact, because stat.txt lists the paths.
  • Evidence: Compare the echo lines at claude-review.yml:138-144 with target.md:7-16.
  • Direction: Add Repository root: $GITHUB_WORKSPACE, Branch or worktree: detached checkout of PR #$NUMBER, and Included paths: see stat.txt.

F3 - Minor - plugins/awf/skills/cr-request/SKILL.md:100

  • Issue: The re-flowed paragraph has a line of about 108 characters. The rest of the file wraps at about 88.
  • Impact: Readability and consistency only.
  • Evidence: Line 100: does not fix, accept, or reject them. A PASS belongs to its target identity; after any change to the target,.
  • Direction: Re-wrap lines 99-101.

Suggestions (optional, not blocking)

  • claude-review.yml Prepare step: --diff-filter=MD leaves out type changes (T). For example, when a file becomes a symlink, base/ gets no merge-base copy of the old file. Consider --diff-filter=MDT. A submodule (gitlink) path would also make git show "$merge_base:$path" fail and abort the step under -eo pipefail. That cannot happen in this repository today.
  • The suggestion from the earlier pass still applies: ci.yml:41 and ci.yml:128 repeat the same metadata: block regex. A short shared comment would keep the two copies from drifting apart.

Review gaps

  • Tests: none were run in this job. That includes the CI Python scripts, the Prepare review target step, and a dry run of the workflow. CI (skill frontmatter, manifests and marketplaces, README versions, hidden Unicode) is reported green on HEAD SHA; that is a signal, not proof.
  • Prepare step and allowlist: the symlink-safety reasoning and the claim that no allowed command can write a file come from reading the code and from what I know of each command's options. Neither was executed here. The PR's claim that it tested the step locally with a committed symlink is unverified by me.
  • Checkout pin: I did not confirm that actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 is the v5.1.0 tag, because the GitHub API was not queried.
  • No live run of the change yet: this PR is reviewed by the workflow on main, which still uses review-request. The cr-request install, the prepared target, and the new allowlist have not run end to end.
  • cr-receive behavior: I reviewed it as instruction text only, and its fit with the cr-request result contract (brief.md:32-53) by reading. No agent was run with the skill.
  • Install claims: the Claude Code and Codex install checks in the description were not reproduced.

Reviewed with: review-request skill

…bump

Resolves yngwald F1 (partly: prompt and comment; the ref bump follows the tag), F2, F3, and the --diff-filter=MDT suggestion.
@yngwald

yngwald commented Oct 18, 2026

Copy link
Copy Markdown
Collaborator

Automated review - PR #2 (fresh review pass)

Target reviewed: committed range 39626fcce7cadd6a77b1cf5ebc6b0ce922f37960...f14da238a8bb4a60c8cff7846e933f4067bf1621. The merge base equals the base SHA 39626fc. There are six commits (b5ac9a3, 30125b9, 1e49986, c21942f, 6902706, f14da23) and 15 files (+383/-50). The working tree is clean, with nothing uncommitted or untracked in scope.

Review mode: fresh review pass. The same reviewer (yngwald) already reviewed the older heads b5ac9a3 and 6902706. I read those comments as context only and did not reuse their verdicts. The new commit since the last pass is f14da23 ("Complete the prepared identity, cover type changes, and note the ref bump"). It touches claude-review.yml and cr-request/SKILL.md. No specialist reviewer was dispatched.

Requirements: the PR title and description (gh pr view 2) and AGENTS.md.

Risk: medium. The change touches the permissions and file writes of an automated CI agent, and it adds a skill made of instruction content.

Coverage: I read the whole diff of the range: ci.yml, claude-review.yml, AGENTS.md, README.md, .claude-plugin/marketplace.json, both plugin manifests, the cr-request SKILL.md, CHANGELOG.md, references/brief.md and references/target.md, and the new cr-receive SKILL.md, references/records.md, CHANGELOG.md and LICENSE. I also checked that the root LICENSE is still tracked.

Verdicts

Axis Verdict
Requirements/spec compliance PASS
Scope compliance PASS
Code/engineering quality FINDINGS
Risk/domain concerns UNASSESSED - static inspection only, see the review gaps
  • Requirements: PASS. Every bullet in the description is in the diff:

    • cr-receive 0.0.1 is added with its intake, anchoring, records, dispositions, fix order, re-review bound and replies;
    • cr-request 0.0.2 hands its result to cr-receive and accepts a prepared target;
    • the auto review installs cr-request from 39626fc and has a Prepare review target step that recreates .auto-review/;
    • git diff, git log and git show are dropped from the allowlist, and merge-base, rev-parse, status and ls-files are kept;
    • both workflows pin checkout to fbc6f39... # v5.1.0;
    • ci.yml reads the version only from the metadata: block;
    • the manifests, marketplace and README are at 0.0.2, and the README License section is removed;
    • the AGENTS.md ref-bump note is added.

    The AGENTS.md rules hold for what can be checked before the tag:

    • cr-receive is listed in the Claude Code manifest, and its name equals the directory;
    • the top CHANGELOG entries match metadata.version (cr-request 0.0.2, cr-receive 0.0.1);
    • the license is MIT, Ihor Orlovskyi;
    • the README shows the repository, plugin and skill versions.
  • Scope: PASS. Every changed file supports one of the described items. I found no unrelated change.

  • Engineering: FINDINGS. There are two Minor items, below. The earlier pass's F2 (incomplete identity), F3 (long line) and the --diff-filter=MDT suggestion are resolved in f14da23. The identity now has all the fields from target.md:7-16.

  • Risk: UNASSESSED. Reading the allowlist, I found no allowed command (git merge-base, rev-parse, status, ls-files, gh pr view, gh pr diff, gh pr checks) that writes to a caller-chosen path. rm -rf .auto-review removes a PR-shipped symlink itself, not what it points to, and every later write goes into the new directory. None of this was executed, so I do not report it as a PASS.

Findings

F1 - Minor - .github/workflows/claude-review.yml:118 (carried from the earlier pass, now documented)

  • Issue: CR_REQUEST_REF still pins cr-request 0.0.1 (39626fc). That version has no prepared-target mode. Its references/target.md runs git log and git diff, and its references/brief.md tells the reviewer to use git show <rev>:<path>. This PR removes all three from the allowlist.
  • Impact: From merge until the ref is bumped, each auto review runs a skill whose instructions conflict with its permissions. The new comment at lines 115-117 and the prompt ("pass the same instruction to any reviewer you dispatch") reduce the risk: the likely cost is some denied tool calls within --max-turns 60. Nothing enforces the bump. It lives in a code comment and in the "After merge" list.
  • Evidence: git show 39626fc:plugins/awf/skills/cr-request/references/target.md has no "Prepared target" section. Commit f14da23 says: "the ref bump follows the tag".
  • Direction: Bump the ref right after tagging v0.0.2, as planned. No further change is needed in this PR.

F2 - Minor - plugins/awf/skills/cr-request/references/target.md:58

  • Issue: The prepared-target contract lists "base copies of modified and deleted files". The workflow now also captures type-changed files (--diff-filter=MDT, claude-review.yml:154), and the prompt describes base/ as "modified, deleted, and type-changed files" (claude-review.yml:185).
  • Impact: The skill's contract drifts from its main caller. A caller that follows target.md alone leaves out type changes, which is exactly the gap that f14da23 closed in CI. Readers of the skill get no merge-base copy of a file that became a symlink.
  • Evidence: target.md:58 compared with claude-review.yml:154 and :185.
  • Direction: Say "modified, deleted, and type-changed files" in target.md:58. It is in this release, so no extra version bump is needed.

Suggestions (optional, not blocking)

  • claude-review.yml:156: use dirname -- "$path". A changed path that starts with - makes dirname read it as an option, and the step aborts under -eo pipefail. This fails closed and is unlikely in this repository.
  • claude-review.yml:135: rm -rf .auto-review also deletes the head side of any PR file under .auto-review/. The prompt says "The checkout is HEAD SHA", but the head side of those paths would then exist only in diff.patch. A one-line note in the prompt, or a ban on tracked .auto-review/ paths, would make this explicit.
  • From the earlier passes: ci.yml:41 and ci.yml:128 repeat the same metadata: block regex. A shared comment would keep the two copies from drifting apart. A submodule path would also make git show "$merge_base:$path" fail. That cannot happen in this repository today.

Review gaps

  • Tests: none were run in this job. That includes the CI Python scripts, the Prepare review target step, and any dry run of the workflow. CI (skill frontmatter, manifests and marketplaces, README versions, hidden Unicode) is reported green on HEAD SHA; that is a signal, not proof.
  • Allowlist and symlink safety: I reasoned from the code and from what I know of each command's options, without executing anything. I did not reproduce the PR's claim that it tested the step locally with a committed symlink.
  • Checkout pin: I did not confirm that fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 is the actions/checkout v5.1.0 tag, because I did not query the GitHub API.
  • No live run yet: this PR is reviewed by the workflow on main, which still uses review-request. The cr-request install, the prepared target and the new allowlist have not run end to end.
  • cr-receive behavior: I reviewed it only as instruction text. I checked its intake against the cr-request result contract by reading. No agent was run with the skill.
  • Install claims and post-merge steps: I did not reproduce the Claude Code and Codex install checks. The tag, the CR_REQUEST_REF bump and the sentimony/skills ref bump are outside this range.

Reviewed with: review-request skill

…pared target

Resolves yngwald F2 and two suggestions of the second pass; F1 stays with the post-tag ref bump.
@yngwald

yngwald commented Oct 18, 2026

Copy link
Copy Markdown
Collaborator

Automated review - PR #2 (fresh review pass)

Reviewed range: 39626fcce7cadd6a77b1cf5ebc6b0ce922f37960...cd82d220669945444aec8c0ec84eeb281bee521c. The merge base is the base SHA 39626fc. The range has seven commits (b5ac9a3, 30125b9, 1e49986, c21942f, 6902706, f14da23, cd82d22) and changes 15 files (+388/-50). The working tree is clean, with nothing staged, unstaged, or untracked in scope.

Review mode: fresh review pass. The same reviewer (yngwald) already reviewed the older heads b5ac9a3, 6902706, and f14da23. I read those comments as context only and did not reuse their verdicts. The only new commit since the last pass is cd82d22 ("Reserve .auto-review, guard dirname, and name type changes in the prepared target"). It touches claude-review.yml and cr-request/references/target.md. No specialist reviewer was dispatched.

Requirements: the PR title and description (gh pr view 2) and AGENTS.md.

Risk: medium. The change touches the permissions and file writes of an automated CI agent, and it adds a skill that consists of instruction content.

Coverage:

  • I read the full diff of cd82d22.
  • I read claude-review.yml at HEAD in full.
  • I re-read the range diff for cr-request (SKILL.md, CHANGELOG.md, references/brief.md, references/target.md), README.md, and the Claude Code manifest.
  • I checked .gitignore.
  • Earlier passes covered ci.yml, AGENTS.md, the marketplace, the Codex manifest, and cr-receive. Those files have not changed since f14da23; I relied on the range diff stat to confirm that, and did not re-read them line by line.

Verdicts

Axis Verdict
Requirements/spec compliance PASS
Scope compliance PASS
Code/engineering quality PASS
Risk/domain concerns FINDINGS - one Minor finding, and the safety analysis is static only (see the review gaps)
  • Requirements: PASS. Every bullet in the description is in the diff:

    • cr-receive 0.0.1;
    • cr-request 0.0.2, with the hand-off to cr-receive and the prepared target;
    • the auto review installs from 39626fc, uses the Prepare review target step, and has the reduced allowlist;
    • the checkout is pinned by SHA;
    • the metadata:-scoped version read in ci.yml;
    • versions at 0.0.2;
    • the README License section is removed;
    • the AGENTS.md ref-bump note is added.

    The AGENTS.md rules that can be checked before the tag hold:

    • cr-receive is listed in the Claude Code manifest skills, and its name equals its directory;
    • the top CHANGELOG entries match metadata.version;
    • the license is MIT, Ihor Orlovskyi;
    • the README shows the repository, plugin, and skill versions.
  • Scope: PASS. cd82d22 addresses exactly the previous pass's F2 and two of its suggestions, as its message states. I found no unrelated change anywhere in the range.

  • Engineering: PASS. The previous pass's findings and suggestions are resolved:

    • F2 is fixed: target.md:57-58 now says "modified, deleted, and type-changed files", which matches claude-review.yml:158 and :189.
    • The dirname -- "$path" suggestion is applied (claude-review.yml:160).
    • The .auto-review/ deletion concern is handled. claude-review.yml:135-138 now fails the job when the PR tracks anything under .auto-review. That pathspec also catches a tracked .auto-review symlink or file, so rm -rf can no longer drop head-side PR content without notice.
  • Risk: FINDINGS. F1 from the earlier passes still applies until the post-tag ref bump. Reading the code, I found no remaining file-write path for the agent:

    • none of the allowed Bash prefixes (git merge-base, rev-parse, status, ls-files, gh pr view, gh pr diff, gh pr checks) writes to a path the caller chooses;
    • the target directory is recreated from scratch, after the reserved-path check.

Findings

F1 - Minor - .github/workflows/claude-review.yml:118 (carried over from earlier passes, documented and planned)

  • Issue: CR_REQUEST_REF still pins cr-request 0.0.1 (39626fc), which has no prepared-target mode. Its references/target.md runs git log and git diff, and its references/brief.md tells the reviewer to use git show <rev>:<path>. This PR removes all three commands from the allowlist (claude-review.yml:208).
  • Impact: From merge until the bump, each auto review runs a skill whose instructions conflict with its tool permissions. The comment at lines 115-117 and the prompt instruction at lines 189-191 reduce the risk, so the likely cost is some denied tool calls within --max-turns 60. Nothing enforces the bump; it exists only in that comment and in the description's "After merge" list.
  • Evidence: git show 39626fc:plugins/awf/skills/cr-request/references/target.md has no "Prepared target" section. The cd82d22 commit message says: "F1 stays with the post-tag ref bump".
  • Direction: bump the ref to the v0.0.2 tag commit right after tagging, as planned. No change is needed in this PR.

Suggestions (optional, not blocking)

  • .gitignore: consider adding .auto-review/. The path is now reserved, and a local run of the workflow or the prompt leaves .auto-review/target/ and comment.md in the working tree. A later git add -A would then commit them and trip the new check at claude-review.yml:135.
  • claude-review.yml:133-134: the comment still gives the symlink case as the reason for the fresh directory. With the reserved-path check in place, rm -rf only clears untracked leftovers. Updating the comment would keep the reasoning accurate.
  • From earlier passes: ci.yml:41 and ci.yml:128 repeat the same metadata: block regex, and a shared comment would keep the two copies from drifting apart. A submodule path would make git show "$merge_base:$path" fail, which cannot happen in this repository today.

Review gaps

  • Tests: none were run in this job. That includes the CI Python scripts, the Prepare review target step (including the new reserved-path check), and any dry run of the workflow. CI (skill frontmatter, manifests and marketplaces, README versions, hidden Unicode) is reported green on HEAD SHA; that is a signal, not proof.
  • Allowlist and symlink safety: I reasoned from the code and from what I know of each command's options, without executing anything. I did not reproduce the PR's claim that it tested the step locally with a committed symlink.
  • Checkout pin: I did not confirm that fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 is the actions/checkout v5.1.0 tag, because I did not query the GitHub API.
  • No live run yet: this PR is reviewed by the workflow on main, which still uses review-request. The cr-request install, the prepared target, the reserved-path check, and the new allowlist have not run end to end.
  • cr-receive behavior: I reviewed it only as instruction text, in earlier passes, and it has not changed since. No agent was run with the skill.
  • Install claims and post-merge steps: I did not reproduce the Claude Code and Codex install checks. The tag, the CR_REQUEST_REF bump, and the sentimony/skills ref bump are outside this range.

Reviewed with: review-request skill

…twin metadata regexes

Applies the three optional suggestions of the third yngwald pass.
@ihororlovskyi
ihororlovskyi merged commit 006400f into main Oct 18, 2026
1 check passed
@ihororlovskyi
ihororlovskyi deleted the 261018-dev-session branch October 18, 2026 14:08
@yngwald

yngwald commented Oct 18, 2026

Copy link
Copy Markdown
Collaborator

Automated review - PR #2 (fresh review pass)

Reviewed range: 39626fcce7cadd6a77b1cf5ebc6b0ce922f37960...efae35bf1f3f630a0e28fce1372f5058b530e37d. The merge base is the base SHA 39626fc. The range has eight commits (b5ac9a3, 30125b9, 1e49986, c21942f, 6902706, f14da23, cd82d22, efae35b) and changes 16 files (+391/-50). The working tree was reported clean at the start of the job.

Review mode: fresh review pass. The same reviewer (yngwald) already reviewed the older heads b5ac9a3, 6902706, f14da23, and cd82d22. I read those comments as context only and did not reuse their verdicts. The only new commit since the last pass is efae35b ("Ignore .auto-review, restate the reserved-path comment, and link the twin metadata regexes"). It touches .gitignore, ci.yml, and claude-review.yml, with comments and one ignore line only. No specialist reviewer was dispatched.

Requirements: the PR title and description (gh pr view 2) and AGENTS.md. No other spec exists.

Risk: medium. The range changes the permissions and file writes of an automated CI agent, and it adds a skill made of instruction content.

Coverage:

  • I read the full diff of efae35b.
  • I read claude-review.yml lines 105-224 at HEAD, which covers the env, the Prepare review target step, the skill install, the agent call with its allowlist, and the start of the post step.
  • I checked the diff stat of the whole range. Earlier passes covered AGENTS.md, README.md, the marketplace, both manifests, cr-request, and cr-receive. Those files have not changed since cd82d22, and I did not re-read them line by line.

Verdicts

Axis Verdict
Requirements/spec compliance PASS
Scope compliance PASS
Code/engineering quality PASS
Risk/domain concerns FINDINGS - one Minor finding, carried over; the safety analysis is static only (see the review gaps)
  • Requirements: PASS. Every bullet in the description is in the diff:

    • cr-receive 0.0.1;
    • cr-request 0.0.2, with the hand-off to cr-receive and the prepared target;
    • the auto review installs cr-request from 39626fc, uses the Prepare review target step, and has the reduced allowlist;
    • checkout is pinned by SHA in both workflows;
    • ci.yml reads the version only from the metadata: block;
    • manifests, marketplaces, and README at 0.0.2, with the README License section removed;
    • the AGENTS.md ref-bump note.

    The AGENTS.md rules that can be checked before the tag still hold, and efae35b changes no skill content, so no version or CHANGELOG bump is needed for it.

  • Scope: PASS. efae35b applies exactly the three optional suggestions from the previous pass, as its message says. The .gitignore line is not named in the description, but it directly supports the reserved .auto-review/ path, so it is a justified supporting change.

  • Engineering: PASS. The new changes are correct as far as static reading shows:

    • .gitignore now ignores .auto-review/. This does not weaken the reserved-path check at claude-review.yml:135: git ls-files lists tracked paths whether or not they are ignored, so a force-added file or a tracked .auto-review symlink still fails the job. As a side effect, the agent's git status no longer shows the prepared target as untracked.
    • The new comment at claude-review.yml:133-134 matches the code: the ls-files check runs first, and rm -rf then clears only untracked leftovers.
    • The cross-reference comments at ci.yml:41 and ci.yml:129 point at each other correctly, and the two regexes are still identical.
  • Risk: FINDINGS. F1 from the earlier passes still applies until the post-tag ref bump. Reading the code, I found no file-write path for the agent: none of the allowed Bash prefixes (git merge-base, rev-parse, status, ls-files, gh pr view, gh pr diff, gh pr checks) writes to a path the caller chooses, and Write/Edit are limited to ./.auto-review/**.

Findings

F1 - Minor - .github/workflows/claude-review.yml:118 (carried over from earlier passes, documented and planned)

  • Issue: CR_REQUEST_REF still pins cr-request 0.0.1 (39626fc), which has no prepared-target mode. Its references/target.md runs git log and git diff, and its references/brief.md tells the reviewer to use git show <rev>:<path>. This PR removes all three commands from the allowlist (claude-review.yml:208).
  • Impact: From merge until the bump, each auto review runs a skill whose instructions conflict with its tool permissions. The comment at lines 115-117 and the prompt at lines 186-191 reduce the risk, so the likely cost is some denied tool calls within --max-turns 60. Nothing enforces the bump; it lives only in that comment and in the description's "After merge" list.
  • Evidence: git show 39626fc:plugins/awf/skills/cr-request/references/target.md has no "Prepared target" section, as recorded in the earlier passes. The comment at claude-review.yml:116-117 says: "Bump it to the v0.0.2 tag commit after release: 0.0.1 lacks the prepared target mode."
  • Direction: Bump the ref to the v0.0.2 tag commit right after tagging, as planned. No change is needed in this PR.

Suggestions (optional, not blocking)

  • The PR description still says the prepare step "recreates .auto-review/ (so PR symlinks are not followed)". The mechanism is now the reserved-path check plus the fresh directory, and .auto-review/ is gitignored. Updating that sentence before squash-merge would keep the merge commit message accurate.
  • From earlier passes: a submodule path would make git show "$merge_base:$path" fail at claude-review.yml:161. That cannot happen in this repository today.

Review gaps

  • Tests: none were run in this job. That includes the CI Python scripts, the Prepare review target step and its reserved-path check, and any dry run of the workflow. CI (skill frontmatter, manifests and marketplaces, README versions, hidden Unicode) is reported green on HEAD SHA; that is a signal, not proof.
  • Allowlist and symlink safety: I reasoned from the code and from what I know of each command's options, without executing anything. I did not reproduce the PR's claim that it tested the step locally with a committed symlink.
  • Checkout pin: I did not confirm that fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 is the actions/checkout v5.1.0 tag, because I did not query the GitHub API.
  • No live run yet: this PR is reviewed by the workflow on main, which still uses review-request. The cr-request install, the prepared target, the reserved-path check, and the new allowlist have not run end to end.
  • cr-receive behavior: reviewed only as instruction text, in earlier passes, and unchanged since. No agent was run with the skill.
  • Install claims and post-merge steps: I did not reproduce the Claude Code and Codex install checks. The tag, the CR_REQUEST_REF bump, and the sentimony/skills ref bump are outside this range.
  • Reviewer independence: this pass was done in one session without a separately dispatched reviewer, so it is a fresh review pass rather than a fully independent review.

Reviewed with: review-request skill

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.

2 participants