feat(CON-8248): draft Connector API changelog entry with an agent - #76
feat(CON-8248): draft Connector API changelog entry with an agent#76jnv wants to merge 9 commits into
Conversation
Completes the regeneration workflow by filling in the commented-out agent step. No new secret is needed: MOXLY_LITELLM_BASE_URL and MOXLY_LITELLM_API_KEY are org-level and surface into any workflow that references them by name, so the "blocked on credentials" note was stale. Stage the regenerated files before the agent runs. The skill's detect script reads `git diff --cached` first, and plain `git diff` would not see a brand-new operations page — the generator writes one page per OpenAPI tag and `config.yaml` filters no tags, so a new tag in the specification produces exactly that. Without staging, the most changelog-worthy change possible was invisible to the agent. Gate both the agent and the pull request on the same staged diff, with `_generator/` excluded because `index.js` rewrites `types.yaml` on every run. A types-cache-only run now exits early instead of opening an empty pull request and burning an agent invocation. This repository is public and the diff originates from a network-fetched OpenAPI specification, so deny egress rather than rely on the input being trusted: no github_token is passed, the allowlist carries no WebFetch/WebSearch/gh/git, and --strict-mcp-config with no --mcp-config guarantees no MCP server is spawned. Edit is granted instead of Write so a confused agent cannot replace a decade of changelog with one entry. Pin the detect script's invocation in the skill itself so the workflow grants a single Bash prefix, and document that untracked files are never detected — the same blind spot applies to interactive use. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
✅ Review posted. View review · run |
moxly
left a comment
There was a problem hiding this comment.
Risk Assessment
Score: 3/10 — low
Scope is a single scheduled workflow plus two skill docs — no runtime or API surface, and every artifact the agent produces lands in a human-reviewed pull request. What keeps this off a 1–2 is the agent step's permission grant: unscoped Edit over a checkout that also contains the one script the agent may execute turns a deliberately narrow Bash allowlist into arbitrary command execution alongside a live gateway key. Likelihood is low (input comes from a first-party spec, no fork-authored trigger path), so the exposure is a hardening gap rather than an active hazard.
Review Summary
Verdict: COMMENT
1 warning, 3 nits.
The staging fix is the right call and the reasoning holds up against the generator: config.yaml sets tags: [] and index.js renders one page per tag into ../operations, so a new OpenAPI tag really does produce an untracked page that plain git diff would miss. saveDiscoveredTypes() running unconditionally likewise justifies excluding _generator/ from the gate. node_modules/ is covered by connector-api/_generator/.gitignore, so git add -A -- connector-api/ stays clean after npm ci. Using steps.changelog.outcome (not conclusion) is correct under continue-on-error.
PR-body claims all check out against the diff: no github_token passed, allowlist limited to Skill,Read,Grep,Glob,Edit plus one Bash prefix, --strict-mcp-config with no --mcp-config, Edit in place of Write, and the skill now pinning the literal repo-root-relative invocation in both SKILL.md and references/git-input-strategy.md. The template is filled with each checklist item justified.
Two things I could not verify from the repo, both already flagged as live-run questions in the PR body: whether GITHUB_ACTIONS: false suppresses every GitHub MCP path in claude-code-action (the cited sibling workflows are not in this repository — this is the only workflow here), and whether the agent's script invocation matches the allowlist prefix exactly. The workflow_dispatch run you recommend before Thursday's cron is worth doing.
Fix All — prompt for AI agent
Fix the following issues in this PR:
- In
.github/workflows/regenerate-connector-api-reference.yml(line 94): the agent is granted unscopedEditplusBashon an in-repo script, so it can rewrite that script and then execute it — arbitrary shell in a job holdingMOXLY_LITELLM_API_KEYand a persistedcontents: writetoken. ScopeEdittoEdit(connector-api/changelog/README.md)andEdit(connector-api/deprecations/README.md); preferably also move the detect-script invocation into a plain workflow step that writes its output to a file outside the repo and drop theBashgrant entirely. Addpersist-credentials: falseto the checkout step andadd-paths: connector-api/**to thecreate-pull-requeststep as defence in depth. - In
.github/workflows/regenerate-connector-api-reference.yml(line 111): make the failure case of the changelog step unmistakable in the PR body instead of a parenthetical — emit a distinct sentence whensteps.changelog.outcome != 'success'. - In
.github/workflows/regenerate-connector-api-reference.yml(line 85): consider guarding against a truncated diff on large regenerations — use--no-diffplus targeted reads, or tell the agent in the prompt to report truncation rather than drafting from a partial view. - In
.claude/skills/connector-api-changelog-entry/references/git-input-strategy.md(line 8): "the form above" points the wrong direction — the invocation forms are below the note. Change to "the form shown below".
After all changes, run the project's linter/formatter.
Scope Edit to the two files the prompt authorises. Unscoped, it reached the detect script itself — the one command the step may also execute — so the agent could rewrite that script and invoke it under the allowlisted prefix, turning a deliberately narrow Bash grant into arbitrary shell next to a live gateway key. This also invalidated the earlier reasoning that unrestricted Read of .git/config was inert: that held only while no arbitrary shell was reachable. Add persist-credentials: false so the contents:write token never lands in .git/config, and add-paths: connector-api/** so an edit elsewhere cannot ride into the commit. create-pull-request pushes with its own token input, so it does not need the persisted credential. Add --path to the detect script and tell the agent to use it. A regeneration diff runs to roughly 60 KB, comfortably past the output limit of the tool reading it, and a truncated diff is indistinguishable from a complete one — so the agent was routinely drafting from a partial view on ordinary weeks, not just large ones. It now lists files with --no-diff and requests them one at a time. The _generator/ exclusion is applied last, so --path can only narrow the scope. Make the failure case of the changelog step unmistakable in the pull request body rather than a parenthetical, since that line is what stops a silent failure passing as a deliberate "no entry needed". Drop the workflow link from git-input-strategy.md: the skill should not know which callers grant it what, only that the invocation is literal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
✅ Review posted. View review · run |
moxly
left a comment
There was a problem hiding this comment.
Risk Assessment
Score: 2/10 — low
Down from 3 on the previous review: the execution-escape gap that kept this off a low score is closed — Edit is now scoped to the two authorised files, the contents: write token is no longer persisted into .git/config, and add-paths fences the commit to connector-api/**. What remains is one bounded hardening gap in the new --path validation, which only widens what the agent can read in a checkout where Read is already unscoped, so it grants no capability the agent lacks today. Still no runtime or API surface, and every artifact lands in a human-reviewed pull request.
Review Summary
Verdict: COMMENT
Re-review scoped to changes since commit b2d751b.
1 warning, 2 nits. 4 resolved — 4 fixed, 0 dismissed; 0 carried forward.
All four findings from the previous review are addressed in code:
- Unscoped
Edit+ allowlistedBashprefix (was a warning) —Editis nowEdit(connector-api/changelog/README.md)andEdit(connector-api/deprecations/README.md)(line 108), so the detect script is no longer editable by the step that may execute it. Both target files exist, and the skill's procedure touches nothing else. All three layers I suggested landed:persist-credentials: falseon checkout (line 24) andadd-paths: connector-api/**on the pull request step (line 117) are the defence-in-depth half. - Failure case buried in a parenthetical — line 128 now branches on
steps.changelog.outcome, emitting a distinct bolded⚠️ sentence with "Write it by hand" instead of asserting the success wording. The&&/||idiom is correct here (the left-hand string is non-empty, so it can't fall through spuriously), andoutcomerather thanconclusionremains the right choice undercontinue-on-error. - Truncated diff on a large regeneration — solved properly rather than papered over:
--pathnarrows the script's output, the two-step--no-diff→ per-file flow is documented inSKILL.md:42andgit-input-strategy.md:22-27, and the prompt now instructs the agent to use it. The--path connector-api/operations/reservations.mdexample points at a file that exists. - "the form above" — reworded to "exactly as written below", and generalised away from naming the workflow, which keeps the skill self-contained. Better than the wording I suggested.
The argument-parsing rewrite is a real improvement over the old grep -v '^--' | head -1: --path/--path= both handled, --path with a missing value rejected, a second positional rejected instead of silently ignored, and the shift accounting is correct (the --path arm shifts once, then falls through to the loop's own shift). Newline as the REQUESTED_PATHS delimiter is sound — newline is IFS whitespace, so the trailing separator produces no empty field. The :(exclude)connector-api/_generator/ pathspec still holds under narrowing, since git applies exclusions regardless of position, so --path connector-api/_generator/types.yaml correctly yields nothing.
One live-run question the PR description doesn't yet list, alongside the two it does: whether Claude Code's permission matcher accepts a bare repo-relative file path in Edit(connector-api/changelog/README.md). If it doesn't match, the agent can read the diff but not write the entry. Like the others it fails visibly — continue-on-error plus the new
Fix All — prompt for AI agent
Fix the following issues in this PR:
- In
.claude/skills/connector-api-changelog-entry/scripts/detect-changed-connector-api-files.sh(lines 41-44): the--pathcheck is a lexical prefix test only, so--path connector-api/../.githubpasses it and then resolves outsideconnector-api/once git normalizes the..in the pathspec. Add a*..*) ... exit 1 ;;arm to thecaseinadd_pathbefore theconnector-api/*arm. Also setset -faround theset -- $REQUESTED_PATHSsplit at line 91 (restoring it after) so a--pathvalue cannot be glob-expanded. Both are needed for the comment at lines 85-86 and the claim atreferences/git-input-strategy.md:57to be accurate as written. - In
.claude/skills/connector-api-changelog-entry/scripts/detect-changed-connector-api-files.sh(line 87):--pathalso narrows the scope in which the "local changes first, else branch diff" priority is evaluated, so a requested path with no working-tree change falls through to the branch diff even when other paths have local changes. Note this in the script'sPriority:help text. - In
.github/workflows/regenerate-connector-api-reference.yml(line 24): no code change needed — add confirming thatcreate-pull-requestpushes successfully with no persisted credential to the list of things to check on the recommendedworkflow_dispatchrun.
After all changes, run the project's linter/formatter.
The check was a lexical prefix test, and git normalizes '..' in a pathspec before matching, so `--path connector-api/../.github/workflows` passed it and then resolved outside the documented scope. Verified against git directly, not just reasoned about. Reject '..' before the prefix test. The split also ran unquoted without `set -f`, so a --path value underwent pathname expansion — `connector-api/../*` widened the pathspec to every top-level entry. Disable globbing across the split. Neither granted the workflow agent a capability it lacked, since Read, Grep and Glob are unscoped there, but both contradicted claims this branch added to the script comments and the reference doc. A claim that is not enforced is worse than no claim. Decide local-versus-branch across the whole of connector-api/ rather than per request. Narrowed per request, a path with no working-tree change fell through to the branch diff while another path still had local changes, so the documented "--no-diff, then --path per file" flow could answer successive calls from two different comparison bases. Fixing it rather than documenting it also retires the temporary file the old two-pass structure needed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
✅ Review posted. View review · run |
moxly
left a comment
There was a problem hiding this comment.
Risk Assessment
Score: 2/10 — low
Unchanged at 2 from the previous review. The three open findings are all addressed, and the priority-scope fix is a genuine simplification rather than a patch — the local-versus-branch decision is now made once for all of connector-api/, and the temp file plus trap are gone with it. What remains is the same bounded class as before: one more way a --path value can escape connector-api/, which only widens what the agent can read in a checkout where Read/Grep/Glob are already unscoped. Still no runtime or API surface, and every artifact lands in a human-reviewed pull request.
Review Summary
Verdict: COMMENT
Re-review scoped to changes since commit d6fb8f1.
1 warning, 1 nit. 3 resolved — 3 fixed, 0 dismissed; 0 carried forward.
All three findings from the previous review are addressed in code:
--pathcontainment was lexical only (was a warning) — the*..*)arm at line 47 lands before the prefix test, andset -f/set +f(lines 104-106) bracket the split, so a value cannot be glob-expanded. Both halves of what I flagged are closed, andgit-input-strategy.md:57was updated in the same change so the reference doc states the actual rule rather than an aspiration.--pathnarrowed the priority check too — fixed at the root instead of documented around: lines 87-95 probe all ofconnector-api/for local changes before any narrowing, so successive--pathcalls can no longer answer from two different comparison bases. ThePriority:help text (lines 31-33) andgit-input-strategy.md:58both say so. This is better than the note I suggested.create-pull-requestpushing with no persisted credential — now the fourth item in the PR description's live-run list, which is all this one asked for. TheEdit(...)permission-matcher question from my last body is in there too.
The restructure holds up on the details. Dropping TMP_FILE removes the mktemp/trap pair with no stale references left, EXCLUDE is quoted at both use sites, the set -f window is exactly the split, and the -eq comparison on USE_LOCAL is sound. The new probe deliberately ignores --path — which is the whole point — and costs two extra git diff --name-only calls per invocation, negligible against the diff work that follows.
Fix All — prompt for AI agent
Fix the following issues in this PR:
- In
.claude/skills/connector-api-changelog-entry/scripts/detect-changed-connector-api-files.sh(lines 44-50): a--pathvalue containing a newline passes thecasevalidation as one string, then splits into two pathspecs at line 105 becauseIFSis a newline — so--path $'connector-api/operations\n.github/workflows'reaches outsideconnector-api/. Add a*"<newline>"*)arm rejecting newlines to thecaseinadd_path, using the same literal-newline-in-quotes idiom as theIFSassignment at lines 102-103. Without it the comment at lines 97-99 and the claim atreferences/git-input-strategy.md:57are still not true as written. - In
.claude/skills/connector-api-changelog-entry/scripts/detect-changed-connector-api-files.sh(line 115): optional — the{ git diff; git diff; } | sort -upipeline takessort's exit status, so agit difffailure no longer aborts underset -ethe way the previous redirect-to-temp-file did. Only worth changing if you want the--no-difflocal path to fail as loudly as the arm below it.
After all changes, run the project's linter/formatter.
Three review rounds found the same class of bug in this file: --path values
escaped their containment through shell mechanics rather than through the check
itself. Git's normalization of '..' defeated a lexical prefix test, pathname
expansion widened 'connector-api/../*' to every top-level entry, and field
splitting on the newline that stood in for an array POSIX sh does not have
turned one validated value into two pathspecs. Each fix was another pattern arm
guarding a symptom of the language choice.
Node removes the class. execFileSync passes argv with no shell, so there is no
field splitting and no globbing to guard, and resolving each value against the
scope root rejects '..' and absolute paths structurally instead of by pattern.
A value containing a newline is now one literal path that matches nothing
rather than two pathspecs. Verified against all three earlier escapes.
execFileSync also throws on a non-zero git exit, which restores the loud
failure the `{ git diff; git diff; } | sort -u` pipeline had swallowed, and
maxBuffer is raised well past the 1 MB default because a full regeneration diff
would otherwise throw on size alone.
Node is already required to work in this repository, so this adds no
dependency, and setup-node already runs before the agent step. The script is
granted twice in the allowlist, bare and node-prefixed: the skill pins the bare
form and the file is executable, but a .mjs path is a strong attractor for a
`node ` prefix and a denied first attempt in an unattended weekly job is not
worth saving one line.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… script Granting both the bare and node-prefixed forms hedged against an agent's choice; pinning one and removing the other is cheaper to reason about. The allowlist now carries a single entry, and the skill, the reference doc and the script's own usage line all show that same command. Drop the shebang and the executable bit so there is no second way to invoke it for the grant to have to cover. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
✅ Review posted. View review · run |
moxly
left a comment
There was a problem hiding this comment.
Risk Assessment
Score: 3/10 — low
Up one from the previous review's 2. The rewrite closes the containment class outright — execFileSync argv plus a resolve-and-compare check is structurally stronger than the pattern arms it replaces, and both prior findings are gone with the shell. What raises the score is a new defect in the same place the old one lived: process.exit(0) after writing the diff to a pipe can drop unflushed output, reintroducing the silent truncation the script's own help text is written to prevent. Still no runtime or API surface, and every artifact lands in a human-reviewed pull request.
Review Summary
Verdict: COMMENT
Re-review scoped to changes since commit bfe73b4.
1 warning. 2 resolved — 2 fixed, 0 dismissed; 0 carried forward.
Both findings from the previous review are fixed in code, and by removal of the language rather than another guard:
- Newline in a
--pathvalue split into two pathspecs (was a warning) — gone.toScopedPathspecreturns one string andexecFileSyncpasses argv with no shell, so a value containing a newline is a single literal path that matches nothing. There is noIFSleft to split on. { git diff; git diff; } | sort -uswallowed git's exit status (was a nit) — gone.execFileSyncthrows on a non-zero git exit, so both the--no-diffand full-diff arms now fail loudly and symmetrically.
The containment check itself holds up. relative(scopeRoot, resolve(repoRoot, value)) cannot return a non-..-prefixed relative path for a target outside scopeRoot, so all three historical escapes are closed structurally: connector-api/../.github normalizes to ../.github, an absolute path yields ../../etc/... (or trips isAbsolute across Windows roots), and connector-api/..* is rejected on the .. prefix before git ever sees it. Git's own pathspec globbing still works within the scope, which the PR body calls out as intended. :(exclude) is appended last at line 119 and applies regardless of order, so the _generator/ half remains sound too. references/git-input-strategy.md:57 now describes that mechanism accurately rather than the pattern-matching it replaced.
The rename is clean — no stale .sh reference survives anywhere in the repo, and the four doc invocations, SKILL.md:38, and the workflow allowlist at line 112 all pin the same literal node <path>.mjs string. actions/setup-node (workflow line 26) does run before the changelog step, so the comment added at lines 80-82 is accurate.
Two earlier-round items are worth noting as settled, since they bear on the new finding: the workflow prompt now steers the agent to --no-diff then --path per file, and the PR body renders a distinct ⚠️ sentence when steps.changelog.outcome != 'success'. The truncation hazard below is the one place that first mitigation does not fully reach.
Fix All — prompt for AI agent
Fix the following issues in this PR:
- In
.claude/skills/connector-api-changelog-entry/scripts/detect-changed-connector-api-files.mjs(lines 134-154):process.exit(0)at line 144 runs immediately after writing the diff to stdout. On Linux,process.stdoutis asynchronous when connected to a pipe — which is how the agent's Bash tool runs this — andprocess.exit()discards pending stdout writes, so any output past the 64 KB pipe buffer is silently dropped with exit status 0. This is the primary CI path: theStage regenerated referencestep stages everything, sohasLocalChangesis always true in the workflow. Restructure the trailingif (hasLocalChanges) { … process.exit(0); }plus the following branch-diff block into a singleif / elseso the process ends naturally and Node flushes before exiting. Leavefail()and the--helppath alone; their payloads are well under the pipe buffer.
After all changes, run the project's linter/formatter.
process.stdout is asynchronous when it is a pipe, which is how the caller reads this script, and process.exit discards writes still queued past the 64 KB pipe buffer. The local-changes branch called it immediately after writing the diff, so output was silently cut at 65536 bytes with exit status 0 — the truncation the Priority notes exist to prevent, reintroduced by the port and reachable on the workflow's only path, since staging makes hasLocalChanges always true there. Measured, not inferred: a 464 KB diff arrived as exactly 65536 bytes through a pipe and 463958 bytes when redirected to a file, which is why the earlier scratch-repo runs never caught it — every diff in them was a few hundred bytes. Fold the two branches into one if/else so the process ends naturally and Node flushes first. fail() and --help keep their exits: both write a single synchronous payload far under the buffer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every review round on this branch found a bug in this script, and each round I verified the fix in a throwaway git repository that was then discarded — so the next round started from nothing and the size dimension went untested entirely until it produced a silent truncation. This makes that work permanent. Fifteen cases, one per bug that actually reached review: the three containment escapes, the _generator exclusion, the untracked-file blind spot, the comparison-base rule, argument-parse rejections, the branch-diff fallback, and output size through a pipe. The script is always invoked via execFileSync, whose stdout is a pipe. That is the point rather than convenience: stdout is asynchronous on a pipe and synchronous to a file, so a file-based test would have passed against the truncating revision. Confirmed load-bearing by running the suite against that revision — 14 pass, 1 fail, and the failure is the pipe case. node:test and node:assert only, no dependencies and no runner config: node --test .claude/skills/connector-api-changelog-entry/scripts/detect-changed-connector-api-files.test.mjs Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
✅ Review posted. View review · run |
moxly
left a comment
There was a problem hiding this comment.
Risk Assessment
Score: 1/10 — low
Down from the previous review's 3. The one warning that review raised — process.exit(0) discarding queued stdout on a pipe — is fixed structurally rather than patched, and the new commit is otherwise test-only: a dependency-free node:test suite that adds no runtime surface and cannot be reached by the workflow's Bash allowlist.
Review Summary
Verdict: APPROVE
Re-review scoped to changes since commit 7a3344b.
1 nit. 1 resolved — 1 fixed, 0 dismissed; 0 carried forward.
The truncation warning is fixed at the source. The trailing if (hasLocalChanges) { … process.exit(0); } plus the follow-on branch-diff block are now a single if / else (lines 138-157), so no process.exit sits between a stdout write and process teardown on either arm. fail() and the --help path keep their exits, and both still write a single small payload before any stdout output, so the flush hazard does not reach them.
The regression suite is well aimed at this script's actual failure history rather than at coverage for its own sake. Two things stand out:
- The pipe case is constructed to fail against the pre-fix revision.
run()goes throughexecFileSyncwith default stdio, so the child's stdout is a pipe (async on Linux); the reference is captured by handing the child anfdfromopenSync, where stdout is synchronous. Comparing the two is the only shape of this test that discriminates — a file-only assertion would have passed against the truncating revision. Thepiped.length > 64 * 1024guard keeps the fixture honest ifbigContentsis ever shrunk, and the fixture clears it comfortably (~180 KB across two files). - The containment cases match the three escapes that actually reached review, and each asserts the mechanism rather than the message:
connector-api/../.github/workflowsand/etcexit 1 viatoScopedPathspec's resolve-and-compare, and the embedded-newline value is asserted to produce no output — which is the correct post-execFileSyncbehaviour (one literal path matching nothing), not merely a rejection.git pathspec globbing still works inside the scopeguards the other direction, so a future "fix" that rejects*outright would be caught.
--path does not switch the comparison base (lines 181-194) is the subtlest one and it is set up correctly: a branch-only change to bills.md plus a working-tree change to reservations.md means a per-path priority decision would report bills.md from BASE_REF...HEAD, and the test asserts the empty result that the whole-scope decision produces.
The new file does not widen the workflow's grant. Bash(node …/detect-changed-connector-api-files.mjs:*) is a prefix match, and …/detect-changed-connector-api-files.test.mjs does not start with it, so the agent cannot invoke the suite; Edit remains scoped to the two changelog files, so it cannot author one either.
The PR body's note that nothing runs this in CI yet is accurate and reads as a deliberate follow-up rather than an oversight — worth tracking, but the suite is runnable as documented and does not depend on a runner.
Fix All — prompt for AI agent
Fix the following issues in this PR:
- In
.claude/skills/connector-api-changelog-entry/scripts/detect-changed-connector-api-files.mjs(lines 134-137): the comment states "Nothing below may call process.exit", butfail()at line 150 does exactly that. The call is safe (it precedes any stdout write), so narrow the comment's wording to "Nothing below may call process.exit once anything has been written to stdout" and note thefail()exception, rather than changing the code.
After all changes, run the project's linter/formatter.
…anged-connector-api-files.mjs Co-authored-by: Moxly <moxly@mews.com>
Summary
CON-8248 – follow-up to #72, which landed the regeneration workflow with the changelog step commented out.
This fills that step in. After the reference is regenerated, an agent runs the existing
connector-api-changelog-entryskill against the fresh diff, decides whether an entry is warranted, and writes it intoconnector-api/changelog/README.md(plusconnector-api/deprecations/README.mdwhen deprecations are involved) before the pull request is opened. Reviewers verify a draft instead of authoring one from a diff.No new secret is required. The
Blocked on credentialsnote was stale:MOXLY_LITELLM_BASE_URLandMOXLY_LITELLM_API_KEYare org-level and surface into any workflow that references them by name, so the step routes through the Moxly LiteLLM gateway with spend attributed viax-litellm-tags. No MCP servers and no additional integrations.Staging fixes a silent blind spot
The regenerated files are now staged before the agent runs. Two reasons:
git diff --cachedfirst, so staging needs no extra plumbing.git diffnever sees untracked files. The generator writes one page per OpenAPI tag andconfig.yamlfilters no tags, so a new tag in the specification creates a new, untrackedconnector-api/operations/*.md. Without staging, the most changelog-worthy change possible – a whole new group of operations – was invisible to the agent.The same blind spot applies to running the skill interactively after a local regeneration, so it is now documented in the skill rather than only worked around in the workflow.
Early exit on a quiet week
Both the agent step and the pull request step are gated on the staged diff with
_generator/excluded, matching the exclusion the skill already uses.index.jsrewrites_generator/types.yamlon every run, so without the exclusion a types-cache-only week would open an empty pull request and burn an agent invocation. Such a week now exits early and the cache change rides along with the next real one.Security posture
This repository is public and the diff originates from a network-fetched OpenAPI specification, so the input is treated as untrusted rather than assumed safe. There is no fork-authored path into the workflow (
scheduleandworkflow_dispatchonly), and the remaining exposure – a live gateway key in the agent's environment – is closed by denying egress:github_tokenis passed. The agent never talks to GitHub;create-pull-requestopens the PR afterwards with the default token.Skill,Read,Grep,Globplus exactly oneBashprefix (nodeon the skill's detect script). NoWebFetch,WebSearch,gh,curl, orgit.--strict-mcp-configwith no--mcp-configguarantees no MCP server is spawned, andGITHUB_ACTIONS: falsestopsclaude-code-actionattaching a GitHub MCP surface by any route that flag does not cover – matching the convention in the sibling Moxly workflows.Editis granted instead ofWrite, and scoped toconnector-api/changelog/README.mdandconnector-api/deprecations/README.md. Unscoped, it would have reached the detect script itself – the one command the step may also execute – so the agent could have rewritten that script and invoked it under the allowlisted prefix, turning a deliberately narrowBashgrant into arbitrary shell. Scoping also makes "edit the tables in place only" a permission rather than a request.persist-credentials: falseon checkout keeps thecontents: writetoken out of.git/config, andadd-paths: connector-api/**fences what can ride into the commit.The agent step is
continue-on-error, because the regeneration pull request is the primary value and a failed draft should not withhold it. The PR body surfaces the step's outcome so a silent failure cannot pass as a deliberate "no entry needed".Skill changes
SKILL.mdandreferences/git-input-strategy.mdnow pin one literal, repo-root-relative invocation of the detect script, so the workflow grants a singleBashprefix and the skill stays the single source of truth for its own path. This also correctsgit-input-strategy.md, which claimed its commands were relative to the skill's directory – wrong guidance for anyone working from the repository root.The script gained a repeatable
--pathoption. A full regeneration diff is around 60 KB, comfortably past the output limit of the tool reading it, and a truncated diff is indistinguishable from a complete one – so the agent would have been drafting from a partial view on ordinary weeks, not just large ones. It now lists files with--no-diffand requests them one at a time. Whether the output comes from local changes or from the branch diff is decided across all ofconnector-api/before narrowing, so successive--pathcalls cannot answer from two different comparison bases.The detect script is now Node rather than POSIX sh
Three review rounds found the same class of bug in the shell version:
--pathvalues escaped their containment check through shell mechanics rather than through the check itself.connector-api/../.github..in a pathspec, so a lexical prefix test never sees itconnector-api/../*Each fix was another pattern arm guarding a symptom of the language choice. Node removes the class instead:
execFileSyncpasses argv with no shell, so there is no field splitting and no globbing to guard, and resolving each value against the scope root rejects..and absolute paths structurally rather than by pattern. A value containing a newline is now one literal path that matches nothing.Two consequences worth noting:
execFileSyncthrows on a non-zero git exit, which restores loud failure where the shell version'ssortpipeline had begun swallowing it, andmaxBufferis raised well past Node's 1 MB default because a full regeneration diff would otherwise throw on size alone.The port also introduced a hazard of its own, now fixed.
process.stdoutis asynchronous when it is a pipe — which is how the caller reads this script — andprocess.exit()discards writes still queued past the 64 KB pipe buffer. The local-changes branch called it right after writing the diff, so a 464 KB diff arrived as exactly 65536 bytes with exit status 0: the precise truncation the script's own notes exist to prevent, on the workflow's only path, since staging makeshasLocalChangesalways true there. The two branches are now oneif / elseso the process ends naturally and Node flushes first.fail()and--helpkeep their exits; both write a single synchronous payload far under the buffer.Node is already required to work in this repository (
connector-api/_generator), andactions/setup-nodealready runs before the agent step, so this adds no dependency. The file has no shebang and is not executable:node <path>is the only invocation, so the allowlist grants exactly one string.Testing steps
Verified locally:
if:gates, andcontinue-on-error.promptfolds to exactly one line, so the trailing text is taken as the slash command's argument.types.yaml-only → skip, untracked new operations page → run...traversal refused,connector-api/../*refused, an embedded newline reduced to one non-matching path that never reaches.github, absolute paths refused, git's own pathspec globbing still working,_generator/still excluded,--pathon an unchanged file returning empty instead of silently switching to the branch diff, argument-parse errors, branch-diff fallback, and exit code 1 on a bad base ref.All of the above is now committed rather than thrown away.
detect-changed-connector-api-files.test.mjsholds 15 cases, one per bug that actually reached review on this branch: the three containment escapes, the_generatorexclusion, the untracked-file blind spot, the comparison-base rule, the argument-parse rejections, the branch-diff fallback, and output size through a pipe.node:testandnode:assertonly — no dependencies, no runner config:The script is always invoked through
execFileSync, whose stdout is a pipe. That is the point rather than convenience: stdout is asynchronous on a pipe and synchronous to a file, so a file-based test would have passed against the truncating revision. Verified load-bearing by running the suite against that revision — 14 pass, 1 fail, and the failure is the pipe case.Nothing runs this in CI yet. The repository has no test job to hang it on, and adding one was outside what this change needed, so it is a follow-up rather than an omission.
Four things only a live run can settle, all non-blocking and all failing loudly:
/connector-api-changelog-entry <args>dispatches the skill as expected. Fallback is plain prose instructing the agent to use the skill.node-prefixed script invocation matches the allowlist prefix now that the skill pins it.Edit(connector-api/changelog/README.md). If it does not match, the agent can read the diff but not write the entry.create-pull-requeststill pushes with no credential persisted in.git/config. It takes its owntokeninput, which is how it supports a token differing from the checkout credential – but if it turns out to rely on the persisted one, the failure mode is this workflow's primary deliverable going missing rather than a degraded one.Recommend one
workflow_dispatchrun before relying on Thursday's cron.continue-on-errormeans the regeneration pull request still lands either way.Checklist
SUMMARY.mdupdated if new pages added – n/a, no new pagesAPI
🤖 Generated with Claude Code