Skip to content

claude: fast triage job plus a build job that can run tests - #16954

Merged
midleman merged 32 commits into
mainfrom
midleman/claude-pr-skill
Oct 28, 2026
Merged

midleman merged 32 commits into
mainfrom
midleman/claude-pr-skill

Conversation

@midleman

@midleman midleman commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Splits @claude into two jobs so questions stay fast and code changes get tested. Addresses #16794.

  • Triage runs first with no dependencies: it answers questions, reviews, and makes docs-only edits
  • Any code change is handed to a build job; triage is blocked from editing code itself
  • Build installs only root + build/ deps (including build/rspack and build/vite, to match the npm-core cache) via a new core value for POSITRON_EXTENSIONS_FILTER; it restores npm-core and never saves a cache
  • Build writes a failing Vitest test first, runs it, and runs eslint and precommit before committing
  • If triage's handoff is missing or malformed, build is skipped and a failure comment is posted

Out of scope / follow-ups

  • Build can't run Core Mocha, extension host, or e2e tests: they need a full compile, Electron, and all extension deps, which would push the job to ~15-20 min. Note: CI still covers them
  • Extension Vitest tests that import that extension's own deps can't run either; a follow-up could install just the touched extension's deps. Do we want that?
  • Neither job can edit .claude/ (the action mounts it read-only), so requests to change skills or rules need a human

QA Notes

Tested on this PR's own runs. The npm-core cache-hit path can only be tested after merge.

  • Question only, build skipped (1m14s): run
  • Code change, build ran with a test-first fix (build 6m05s, cache miss): run, PR #16962
  • Triage blocked from code edits and file-changing git commands: run
  • Missing handoff fails loudly (red on purpose): run

@positron-projects

positron-projects Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @midleman's task in 27s —— View job


Review: 2 findings

The current commit only removes the npm ci path. The triage/build split, the core filter, and the npm-core cache from the PR body are not in the diff yet. I expected this because the PR is WIP.

1. The prompt contradicts itself on tests. .github/workflows/claude.yml:203-204 tells Claude that it cannot run tests. Lines 213-217 still tell it to commit "after each logical, verified increment (for example, once backend tests pass, and again once UI tests pass)". Claude can read that as an instruction to try tests. Remove the test examples from that paragraph, or change them to "after each logical increment". Fix this →

2. --allowedTools still allows tools that cannot work. Line 237 keeps Bash(npm run:*), Bash(npx vitest:*), and Bash(npx eslint:*). Without node_modules, these commands fail, and npx can try to download packages. Remove them from the triage job. Add them only to the new build job.

I did not run anything. This PR changes only a workflow file.

@github-actions

github-actions Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

E2E Tests 🚀
This PR will run tests tagged with: @:critical

Why these tags?

Test count: Linux: 62 tests.

Note

No feature tags detected. If this PR needs feature coverage, add the tag above and retrigger the workflow.

readme  valid tags

@positron-projects

positron-projects Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @midleman's task in 50s —— View job


Review: 2 findings

Commit 2ea50e81 fixed the 2 earlier findings. The core filter in build/npm/dirs.ts is correct. buildSqliteServerBinding already skips a missing extensions/ dir, and the root and build/ installs supply everything the ESM bundling step needs. No job uses the filter yet. That is expected, because the PR is WIP.

1. The JSON schema does not tell Claude what the fields mean. .github/workflows/claude.yml:236 requires needs_build, summary, files, and findings. The schema and the prompt do not define them. Claude must guess when needs_build is true, and the triage-to-build handoff depends on that value. Add a description to each property, for example "true if the task changes code and needs Vitest, eslint, or precommit". Fix this →

2. The SPIKE step can make a good run show the confused reaction. Lines 240-246 pipe structured_output to jq .. If the output is not valid JSON, jq fails and the job status becomes failure. The next step then posts confused, even when Claude finished its task. Add || true after jq . while the spike is in place, or remove the step before merge, as the commit title says.

I did not run anything. This PR changes only a workflow file and a build script.

@positron-projects

positron-projects Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @midleman's task in 50s —— View job


Review: 1 finding

The two findings from the last review are fixed. The build job is not in the diff yet. I expected this because the PR is WIP.

1. The core filter does not install the extension dependencies that some Vitest tests need. build/npm/dirs.ts:201 keeps only the root and build/. vitest.config.ts:16 also collects extensions/**/*.vitest.{ts,tsx}. If one of those tests imports a package from its extension's node_modules, the test fails with a "module not found" error in the build job. Claude can read that error as a real failure. When you add the build job prompt, tell Claude that only src/vs Vitest tests can run there. Fix this →

Note: the SPIKE print structured output step and the --json-schema argument are temporary. Remove them before merge, as the commit message says.

I did not run anything. This PR changes only a workflow file and a build script.

@positron-projects

positron-projects Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @midleman's task in 1m 11s —— View job


Review: 2 findings

The new composite actions look correct. On a cache hit, restore-build-caches supplies a superset of what the core filter installs. The core filter also drops remote/reh-web, so postinstall does not need gulp-merge-json, and skipping npm --prefix build ci is safe. The triage/build handoff is not in the diff yet. I expected this because the PR is WIP.

1. The spike-sandbox job runs on every PR push, without the write-permission check. .github/workflows/claude.yml:252 uses only github.event_name == 'pull_request'. The main job checks the actor first, but this job does not. On fork PRs, OP_SERVICE_ACCOUNT_TOKEN is empty, so the job fails. On other PRs, the job calls the API on each push. Remove the job before merge, as its comment says. Until then, add the same actor check, or limit the job to this branch.

2. The core filter also installs build/rspack and build/vite. build/npm/dirs.ts:201 keeps d.startsWith('build/'). Vitest, eslint, and build/hygiene.ts do not use these packages, and they pull in @rspack/*, vite, and rollup. On a cache miss, this adds install time for no use. Change the filter to d === '' || d === 'build', and update the comment at line 158. Fix this →

I did not run anything. This PR changes only workflow files, composite actions, and a build script.

@positron-projects

positron-projects Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @midleman's task in 28s —— View job


Review: no new findings

The new commit 85470f0a adds only the Print tool results spike step. The jq filter reads the execution_file message format correctly.

Two findings from the last review are still open:

  1. spike-sandbox (.github/workflows/claude.yml:252) has no actor check. It runs on every PR push and fails on fork PRs. Remove the job before merge, or limit it to this branch until then.
  2. The core filter (build/npm/dirs.ts:201) also installs build/rspack and build/vite. Change it to d === '' || d === 'build'. Fix this →

The triage/build handoff is not in the diff yet. I expected this because the PR is WIP.

I did not run anything. This PR changes only workflow files, composite actions, and a build script.

@positron-projects

positron-projects Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @midleman's task in 1m 9s —— View job


Review: 1 new finding

The triage/build split in ab8aa705 looks correct. The handoff step fails closed: empty or malformed output posts a comment, sets needs_build=false, and posts the confused reaction. The two jobs do not post the reaction twice. The spike jobs and steps are gone.

1. On a PR, the build job installs the dependencies of main, not of the PR branch. For comment events, actions/checkout checks out main. setup-claude-build-deps then computes the npm-core cache key and runs npm ci against the lockfiles of main. claude-code-action switches to the PR head branch only after that step. If the PR adds or changes a dependency, Vitest or eslint fails with "module not found" or uses the wrong version. Claude can read that error as a real failure. To fix this, check out the PR head before setup-claude-build-deps when the event is on a PR. Alternatively, tell Claude in the build prompt that the dependencies come from main, so a changed package.json can cause install errors. Fix this →

Still open from the last review: the core filter (build/npm/dirs.ts:201) also installs build/rspack and build/vite.

I did not run anything. This PR changes only workflow files, composite actions, and a build script.

@positron-projects

positron-projects Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @midleman's task in 20s —— View job


Handed off: uppercase backend schemes in PositronDataExplorerUri.parse

I did not edit any files. The request changes a file under src/ and a file under .claude/. For this reason, I handed off the whole request to the second job.

  • Read the request
  • Find the cause
  • Hand off to the build job

Cause. The regex at src/vs/workbench/services/positronDataExplorer/common/positronDataExplorerUri.ts:45 accepts only lowercase scheme characters: ([a-z][a-z0-9+.-]*:.+). For this reason, DuckDB:/tmp/a.csv does not match, and parse returns undefined.

Fix for the build job.

  1. Change the scheme part of the regex to [a-zA-Z][a-zA-Z0-9+.-]*.
  2. Add cases for uppercase schemes to src/vs/workbench/services/positronDataExplorer/test/common/positronDataExplorerUri.vitest.ts.
  3. Add one line to the end of .claude/rules/vitest-tests.md.
  4. Create the branch claude/spike-case2 from origin/main, and open a draft PR to main.

Note. backingUri at line 70 uses startsWith('duckdb:'), which is case-sensitive. After the fix, parse returns DuckDB:/tmp/a.csv, but backingUri still returns undefined for it. The build job must decide whether to compare the scheme without case.

A second job continues the work: build job run.

@positron-projects

positron-projects Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @midleman's task in 17s —— View job


Handed off: uppercase backend schemes in PositronDataExplorerUri.parse

I did not edit any files. The request changes a file under src/ and a file under .claude/. For this reason, I handed off the whole request to the build job.

  • Read the request
  • Find the cause
  • Hand off to the build job

Cause. The regex at src/vs/workbench/services/positronDataExplorer/common/positronDataExplorerUri.ts:45 accepts only lowercase scheme characters: ([a-z][a-z0-9+.-]*:.+). For this reason, DuckDB:/tmp/a.csv does not match, and parse returns undefined.

Steps for the build job.

  1. Change the scheme part of the regex to [a-zA-Z][a-zA-Z0-9+.-]*.
  2. Add test cases for uppercase schemes to src/vs/workbench/services/positronDataExplorer/test/common/positronDataExplorerUri.vitest.ts.
  3. Add one line to the end of .claude/rules/vitest-tests.md.
  4. Create the branch claude/spike-case2 from origin/main, and open a draft PR to main.

Note. backingUri at line 70 uses startsWith('duckdb:'), which is case-sensitive. After the fix, parse returns DuckDB:/tmp/a.csv, but backingUri still returns undefined for it. The build job must decide whether the scheme comparison ignores case.

A second job continues the work: build job run.

@positron-projects

positron-projects Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @midleman's task in 2m 41s —— View job


Opened draft PR #16956: uppercase backend schemes in PositronDataExplorerUri

parse now accepts identifiers such as DuckDB:/tmp/a.csv. backingUri now compares the duckdb: prefix without case. I did not push to this PR's branch.

  • Create claude/spike-case2 from origin/main
  • Write a failing test (2 new tests failed for the expected reason)
  • Fix the regex and backingUri (4 of 4 tests pass, precommit passes)
  • Add one line to .claude/rules/vitest-tests.md: not done
  • Open a draft PR, add @midleman as reviewer and assignee

Not done: the .claude/ edit. .claude/ is a read-only mount in this job, so writes fail with EROFS. I did not try to bypass the mount. .github/ is read-only too. If the build job must edit docs under .claude/, change the mount. Otherwise, tell the triage job to reject those edits.

Problem in the build job setup. .claude/ stays read-only when you switch branches. To make a branch from origin/main, I used git reset and restored the other files. git checkout failed because it must rewrite files under .claude/. The working tree under .github/ still contains the files of the PR branch, but they are not staged.
· Branch claude/spike-case2

@positron-projects

positron-projects Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @midleman's task in 10s —— View job


Answer: PositronDataExplorerUri.backingUri returns undefined for a runtime comm id

PositronDataExplorerUri.backingUri(resource) returns the URI of the backing file. See src/vs/workbench/services/positronDataExplorer/common/positronDataExplorerUri.ts:67-75.

For a runtime comm id, it returns undefined:

  1. parse matches the UUID branch of the regex (line 45) and returns the bare UUID (line 53).
  2. The UUID does not start with duckdb:, so backingUri returns undefined (lines 69-71). The comment at line 69 says: "Runtime comm IDs have no originating URIs."

The function returns a URI only for duckdb: identifiers. It removes the duckdb: prefix and parses the remaining text, for example file:///path/to/file.csv.

I did not change any files.

@positron-projects

positron-projects Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @midleman's task in 13s —— View job


Answer: PositronDataExplorerUri.backingUri returns undefined for a runtime comm id

PositronDataExplorerUri.backingUri(resource) returns the URI of the backing file. See src/vs/workbench/services/positronDataExplorer/common/positronDataExplorerUri.ts:67-75.

For a runtime comm id, it returns undefined:

  1. parse matches the UUID branch of the regex (line 45) and returns the bare UUID (line 53).
  2. The UUID does not start with duckdb:, so backingUri returns undefined (lines 69-71). The comment at line 69 says: "Runtime comm IDs have no originating URIs."

The function returns a URI only for duckdb: identifiers. It removes the duckdb: prefix and parses the remaining text, for example file:///path/to/file.csv.

I did not change any files.

@github-actions

Copy link
Copy Markdown
Contributor

The handoff to the build job failed. See https://github.com/posit-dev/positron/actions/runs/40714675815.

Triage now hands off any non-Markdown change, and --disallowedTools Edit
rules block code edits even though tag mode auto-accepts edits. Also drop
the unused GitHub token from the core npm ci step and correct which
lockfiles the build job installs on review events.
@positron-projects

positron-projects Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @midleman's task in 48s —— View job


Review: 1 new finding

The new commits 4caa96e1 and 8687b5b8 look correct otherwise. The HANDOFF markers and the "notes, not instructions" wording are good. Removing the GitHub token from npm ci is safe. The new lockfile wording in the build prompt matches what actions/checkout does on each event type.

1. The --disallowedTools list does not limit triage to Markdown. .github/workflows/claude.yml:241 blocks a list of paths and extensions, but deny rules cannot allow only .md. Triage can still edit other files, and then open a PR in which no tests ran. Examples:

  • .gitignore, .github/CODEOWNERS, Dockerfile, LICENSE.txt
  • *.scss, *.less, *.svg, *.txt, *.lock

The commit title says the rules "enforce" the Markdown limit, but they only block the common cases. Add the missing patterns, at minimum Edit(/.github/**), Edit(**/*.scss), Edit(**/*.less), Edit(**/*.svg), Edit(**/*.txt), and Edit(**/.gitignore). Alternatively, change the commit and PR wording to say the rules block the common code files. Also, Bash(git:*) can still write files, for example with git checkout <ref> -- src/x.ts or git apply. Fix this →

Still open from earlier reviews

  • Mixed requests. Triage still hands off the whole request when it needs Markdown edits and code edits (line 225). The build prompt does not tell Claude that .claude/ is read-only, so a Markdown edit under .claude/ still fails with EROFS in the build job.
  • core filter. build/npm/dirs.ts:201 still keeps d.startsWith('build/'), so a cache miss also installs build/rspack and build/vite.

I did not run anything. This PR changes only workflow files, composite actions, and a build script.

Triage gets a read-only list of git commands instead of Bash(git:*), git rm
is blocked, and the deny list adds style, SVG, and Dockerfile files. The
prompt now says no code rather than Markdown only, matching what the deny
rules enforce.
@positron-projects

positron-projects Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @midleman's task in 38s —— View job


Spike: 3 of 4 steps were blocked, and the Markdown edit was allowed

The permission rules worked as planned. The code edit and both file-changing git commands were blocked. Only the README.md edit was allowed.

Step Result
1. Edit src/vs/workbench/services/positronDataExplorer/common/positronDataExplorerUri.ts Denied: File is in a directory that is denied by your permission settings.
2. git checkout HEAD~0 -- package.json Denied: This command requires approval
3. git rm --cached README.md Denied: Permission to use Bash with command git rm --cached README.md has been denied.
4. Edit README.md Allowed. The line spike was added.

Note on step 2. Step 2 was not blocked by a deny rule. It failed because the command is not on the allow list, and nobody can approve a prompt in CI. The result is the same, but the error is different from step 3. If you want the same explicit denial, add Bash(git checkout:*) to --disallowedTools.

Cleanup. After step 4, I removed the spike line from README.md. git status shows no changes to tracked files. I did not commit, push, or open a PR. needs_build is false.

@positron-projects

positron-projects Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @midleman's task in 44s —— View job


Review: no new findings

The new commit c90ea83b fixes the Bash(git:*) finding. Triage now gets only read-only git commands, and the deny list adds style, SVG, and Dockerfile files. The spike in f12dd67c is reverted, and its result matched the plan.

Small gap. The prompt at .github/workflows/claude.yml:221 says that config files are blocked "anywhere". The deny rules do not block .gitignore, .github/CODEOWNERS, .editorconfig, or *.txt. Claude follows the prompt, so the risk is low. If you want the rules to match the prompt, add Edit(**/.gitignore), Edit(/.github/**), and Edit(**/.editorconfig).

Still open from earlier reviews

  1. Mixed requests. Line 226 still tells triage to hand off the whole request when it needs code edits and doc edits. The build job mounts .claude/ read-only, and the build prompt does not say so. A doc edit under .claude/ fails there with EROFS, as in the Case 2 run. Tell triage to make the doc edits itself and hand off only the code part. Or tell the build job that .claude/ is read-only. Fix this →
  2. core filter. build/npm/dirs.ts:201 still keeps d.startsWith('build/'), so a cache miss also installs build/rspack and build/vite. Change the filter to d === '' || d === 'build'. Fix this →

I did not run anything. This PR changes only workflow files, composite actions, and a build script.

@positron-projects

positron-projects Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @midleman's task in 20s —— View job


Handed off: uppercase backend schemes in PositronDataExplorerUri.parse

I did not edit any files. The request changes a file under src/ and a file under .claude/. For this reason, I handed off the whole request to the build job.

  • Read the request
  • Find the cause
  • Hand off to the build job

Cause. The regex at src/vs/workbench/services/positronDataExplorer/common/positronDataExplorerUri.ts:45 accepts only lowercase scheme characters: ([a-z][a-z0-9+.-]*:.+). For this reason, DuckDB:/tmp/a.csv does not match, and parse returns undefined.

Steps for the build job.

  1. Change the scheme part of the regex to [a-zA-Z][a-zA-Z0-9+.-]*.
  2. Add test cases for uppercase schemes to src/vs/workbench/services/positronDataExplorer/test/common/positronDataExplorerUri.vitest.ts.
  3. Add one line to the end of .claude/rules/vitest-tests.md.
  4. Run git fetch --depth 1 origin main, create the branch claude/spike-case2b from origin/main, and open a draft PR to main.

Note. backingUri at line 70 uses startsWith('duckdb:'), which is case-sensitive. After the fix, parse returns DuckDB:/tmp/a.csv, but backingUri still returns undefined for it. The build job must decide whether the scheme comparison ignores case.

A second job continues the work: build job run.

@positron-projects

positron-projects Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @midleman's task in 2m 25s —— View job


Opened draft PR #16962: uppercase backend schemes in PositronDataExplorerUri

parse now accepts identifiers such as DuckDB:/tmp/a.csv. backingUri now compares the duckdb: prefix without case. I did not push to this PR's branch.

  • Create claude/spike-case2b from origin/main
  • Write a failing test (2 new tests failed with expected undefined)
  • Fix parse and backingUri (4 of 4 tests pass, precommit passes)
  • Add one line to .claude/rules/vitest-tests.md: not done
  • Open a draft PR, add @midleman as reviewer and assignee

Not done: the .claude/ edit. The Edit tool denied the write: "requested permissions to edit ... which is a sensitive file". I did not try to bypass it. If the build job must edit .claude/, allow it there. Otherwise, tell triage not to hand off those edits.

Problem in the build job setup. git switch -c claude/spike-case2b origin/main failed. The job cannot remove the two files under .github/actions/ that exist only on the PR branch. I made the branch with git branch, git symbolic-ref HEAD, and git reset. The working tree still has the PR branch version of .github/workflows/claude.yml, but it is not staged or committed. Also, git branch could not write the upstream config (could not lock config file .git/config).

Note. positronDataExplorerInstance.ts:728 checks clientId.startsWith('duckdb:'), which is also case-sensitive. I did not change it, because the request did not cover it.
· Branch claude/spike-case2b

@positron-projects

positron-projects Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @midleman's task in 27s —— View job


Review: no new findings

The new commits f12dd67c through eb6230fe are 2 spikes and their reverts. The net diff since c90ea83b is empty. The Case 2 rerun matches the QA notes in the PR body.

Still open from earlier reviews

  1. Mixed requests. The PR body says that neither job can edit .claude/. But .github/workflows/claude.yml:226 still tells triage to hand off the whole request when it needs code edits and doc edits. The build prompt does not say that .claude/ is read-only. In both Case 2 runs, the build job tried the .claude/ edit and reported it as not done. Tell triage to reject edits under .claude/ and to hand off only the code part. Add one sentence to the build prompt that .claude/ is read-only. Fix this →
  2. core filter. build/npm/dirs.ts:201 still keeps d.startsWith('build/'). On a cache miss, the job also installs build/rspack and build/vite. Change the filter to d === '' || d === 'build'. Fix this →

I did not run anything. This PR changes only workflow files, composite actions, and a build script.

@positron-projects

positron-projects Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @midleman's task in 28s —— View job


Review: no new findings

The new commit c6ff4d7a is correct. The pull_request trigger now also runs when setup-claude-sandbox or setup-claude-build-deps changes. Both paths match the action directories in this PR.

Still open from earlier reviews

  1. The prompts do not match the PR body about .claude/. The PR body says that neither job can edit .claude/, and that a human must make those changes. But .github/workflows/claude.yml:227-228 still tells triage to "hand off the whole request" for mixed code and doc requests. The build prompt does not say that .claude/ is read-only. In run 40723658820, the build job tried the .claude/ edit and failed. Tell triage to report .claude/ edits as not possible, and hand off only the code part. Fix this →
  2. core filter. build/npm/dirs.ts:201 still keeps d.startsWith('build/'). On a cache miss, the job also installs build/rspack and build/vite. Change the filter to d === '' || d === 'build'. Fix this →

I did not run anything. This PR changes only workflow files, composite actions, and a build script.

@positron-projects

positron-projects Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @midleman's task in 28s —— View job


Review: 1 open finding

The new commit 3862256c fixes the .claude/ finding. The sentence is in SHARED_PROMPT (.github/workflows/claude.yml:39-40), so both jobs get it. If triage hands off a mixed request, the build job now skips the .claude/ part and reports it as not done. The PR body says the same thing.

Still open from earlier reviews

  1. core filter. build/npm/dirs.ts:201 still keeps d.startsWith('build/'). On a cache miss, the job also installs build/rspack and build/vite. Vitest, eslint, and precommit do not use them. Change the filter to d === '' || d === 'build', and update the comment at line 158. Fix this →

I did not run anything. This PR changes only workflow files, composite actions, and a build script.

@positron-projects

positron-projects Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @midleman's task in 15s —— View job


Review: no open findings

The new commit 009b0ad3 closes the last open finding. The core filter in build/npm/dirs.ts:201 still keeps build/rspack and build/vite. The comment at lines 158-159 and the log message now say so and give the reason: the subfolders match the npm-core cache. This is a valid choice. On a cache hit, the job restores those folders anyway.

All earlier findings are closed.

I did not run anything. This PR changes only workflow files, composite actions, and a build script.

@midleman
midleman marked this pull request as ready for review October 28, 2026 16:42

@nealrichardson nealrichardson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems directionally right to me, worth trying out. Left a couple of suggestions.

Comment thread .github/workflows/claude.yml Outdated
token: ${{ steps.generate-token.outputs.token }}
persist-credentials: false

- name: Checkout Simple English plugin

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to pull more of these steps into the shared setup-claude-sandbox and make that a broader setup-claude action?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 265f901. setup-claude-sandbox is now setup-claude, and it runs the Simple English plugin checkout, the Anthropic key load, the sandbox setup, and Node setup for both jobs. The token and repo checkout stay in the workflow, since a local action needs the repo checked out first. This run passed with it.

--fallback-model sonnet
--allowedTools "Edit,Write,WebFetch(domain:posit.co),WebFetch(domain:*.posit.co),WebSearch,Read,Skill,Bash(git:*),Bash(gh pr view:*),Bash(gh pr edit:*),Bash(gh issue view:*),Bash(gh issue edit:*),Bash(gh label list:*),Bash(gh run view:*),Bash(npm ci:*),Bash(npm --prefix build ci:*),Bash(npm run:*),Bash(npx vitest:*),Bash(npx eslint:*),Bash(node scripts/format.mts:*),Bash(./scripts/file-origin.sh:*),Bash(.claude/skills/positron-pr-helper/scripts/fetch-test-tags.sh:*),mcp__github__create_pull_request,mcp__github__create_issue,mcp__github__search_issues,mcp__github__update_issue,mcp__github__create_pending_pull_request_review,mcp__github__add_comment_to_pending_review,mcp__github__submit_pending_pull_request_review,mcp__github__get_pull_request_diff,mcp__github__add_sub_issue"
--allowedTools "Edit,Write,WebFetch(domain:posit.co),WebFetch(domain:*.posit.co),WebSearch,Read,Skill,Bash(git log:*),Bash(git show:*),Bash(git diff:*),Bash(git blame:*),Bash(git status:*),Bash(git grep:*),Bash(git ls-files:*),Bash(git rev-parse:*),Bash(git fetch:*),Bash(gh pr view:*),Bash(gh pr edit:*),Bash(gh issue view:*),Bash(gh issue edit:*),Bash(gh label list:*),Bash(gh run view:*),Bash(./scripts/file-origin.sh:*),Bash(.claude/skills/positron-pr-helper/scripts/fetch-test-tags.sh:*),mcp__github__create_pull_request,mcp__github__create_issue,mcp__github__search_issues,mcp__github__update_issue,mcp__github__create_pending_pull_request_review,mcp__github__add_comment_to_pending_review,mcp__github__submit_pending_pull_request_review,mcp__github__get_pull_request_diff,mcp__github__add_sub_issue"
--disallowedTools "Edit(/src/**),Edit(/extensions/**),Edit(/build/**),Edit(/scripts/**),Edit(/test/**),Edit(/cli/**),Edit(/remote/**),Edit(/positron/**),Edit(/.eslint-plugin-local/**),Edit(/ai-lib/**),Edit(**/*.ts),Edit(**/*.tsx),Edit(**/*.mts),Edit(**/*.js),Edit(**/*.mjs),Edit(**/*.cjs),Edit(**/*.json),Edit(**/*.rs),Edit(**/*.py),Edit(**/*.R),Edit(**/*.sh),Edit(**/*.ps1),Edit(**/*.cmd),Edit(**/*.toml),Edit(**/*.yml),Edit(**/*.yaml),Edit(**/*.css),Edit(**/*.scss),Edit(**/*.less),Edit(**/*.svg),Edit(**/*.html),Edit(**/Dockerfile*),Edit(/.npmrc),Edit(/.nvmrc),Edit(/.python-version),Bash(git rm:*)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be simpler to express this as an allowlist under allowedTools, rather than allow all Edit and then block these paths?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried an allowlist first, but it doesn't work with this action.

claude-code-action always starts Claude in acceptEdits mode (source). In that mode every file edit in the repo is approved automatically, and --allowedTools never gets checked for edits. So an allowlist of Edit(**/*.md) would still let triage edit a .ts file.

The one thing that mode still respects is --disallowedTools, which is why I went with a blocklist. I tested it in this run: the .ts edit was blocked and the README edit went through.

The downside is that a blocklist can miss a file type we didn't think of. The prompt also tells triage not to edit code, as a second layer.

Both jobs ran the same plugin checkout, secret load, sandbox setup, and
Node setup. The setup-claude-sandbox action becomes setup-claude and
runs all four.
@positron-projects

positron-projects Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @midleman's task in 19s —— View job


Review: no new findings

The new commit 265f9013 answers the review comment from @nealrichardson about a broader setup action. It moves 4 shared steps from both jobs into .github/actions/setup-claude: the plugin checkout, the secret load, the sandbox setup, and the Node setup.

The move is correct:

  • Both jobs now read the key from steps.setup.outputs.anthropic-key. No step still uses steps.load_secrets.
  • The pull_request paths filter now uses the new action folder. No file still uses the old setup-claude-sandbox path.
  • In the build job, Node setup still runs before setup-claude-build-deps.

Still open: the second review comment from @nealrichardson, which asks for an Edit allowlist in place of a deny list. This commit does not change that part.

I did not run anything. This PR changes only workflow files, composite actions, and a build script.

@midleman
midleman enabled auto-merge (squash) October 28, 2026 20:09
@midleman
midleman merged commit 4383114 into main Oct 28, 2026
45 checks passed
@midleman
midleman deleted the midleman/claude-pr-skill branch October 28, 2026 20:44
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 28, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants