Skip to content

feat(CON-8248): draft Connector API changelog entry with an agent - #76

Open
jnv wants to merge 9 commits into
mainfrom
con-8248-changelog
Open

feat(CON-8248): draft Connector API changelog entry with an agent#76
jnv wants to merge 9 commits into
mainfrom
con-8248-changelog

Conversation

@jnv

@jnv jnv commented Aug 7, 2026

Copy link
Copy Markdown
Member

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-entry skill against the fresh diff, decides whether an entry is warranted, and writes it into connector-api/changelog/README.md (plus connector-api/deprecations/README.md when 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 credentials note was stale: MOXLY_LITELLM_BASE_URL and MOXLY_LITELLM_API_KEY are org-level and surface into any workflow that references them by name, so the step routes through the Moxly LiteLLM gateway with spend attributed via x-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:

  • The skill's detect script reads git diff --cached first, so staging needs no extra plumbing.
  • Plain git diff never sees untracked files. The generator writes one page per OpenAPI tag and config.yaml filters no tags, so a new tag in the specification creates a new, untracked connector-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.js rewrites _generator/types.yaml on 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 (schedule and workflow_dispatch only), and the remaining exposure – a live gateway key in the agent's environment – is closed by denying egress:

  • No github_token is passed. The agent never talks to GitHub; create-pull-request opens the PR afterwards with the default token.
  • The allowlist grants Skill,Read,Grep,Glob plus exactly one Bash prefix (node on the skill's detect script). No WebFetch, WebSearch, gh, curl, or git.
  • --strict-mcp-config with no --mcp-config guarantees no MCP server is spawned, and GITHUB_ACTIONS: false stops claude-code-action attaching a GitHub MCP surface by any route that flag does not cover – matching the convention in the sibling Moxly workflows.
  • Edit is granted instead of Write, and scoped to connector-api/changelog/README.md and connector-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 narrow Bash grant into arbitrary shell. Scoping also makes "edit the tables in place only" a permission rather than a request.
  • persist-credentials: false on checkout keeps the contents: write token out of .git/config, and add-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.md and references/git-input-strategy.md now pin one literal, repo-root-relative invocation of the detect script, so the workflow grants a single Bash prefix and the skill stays the single source of truth for its own path. This also corrects git-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 --path option. 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-diff and requests them one at a time. Whether the output comes from local changes or from the branch diff is decided across all of connector-api/ before narrowing, so successive --path calls 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: --path values escaped their containment check through shell mechanics rather than through the check itself.

Escape Cause
connector-api/../.github git normalizes .. in a pathspec, so a lexical prefix test never sees it
connector-api/../* shell pathname expansion
a value containing a newline field splitting on the newline that stood in for an array POSIX sh does not have

Each fix was another pattern arm guarding a symptom of the language choice. Node removes the class instead: 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 rather than by pattern. A value containing a newline is now one literal path that matches nothing.

Two consequences worth noting: execFileSync throws on a non-zero git exit, which restores loud failure where the shell version's sort pipeline had begun swallowing it, and maxBuffer is 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.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 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 makes hasLocalChanges always true there. The two branches are now 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.

Node is already required to work in this repository (connector-api/_generator), and actions/setup-node already 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:

  • Workflow YAML parses with the intended step order, if: gates, and continue-on-error.
  • prompt folds to exactly one line, so the trailing text is taken as the slash command's argument.
  • Gate logic exercised in a scratch repository: clean tree → skip, types.yaml-only → skip, untracked new operations page → run.
  • Detect script exercised in a scratch repository against every case, including all three historical escapes: .. 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, --path on 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.
  • Output size through a pipe, which the cases above were all too small to test: a 464 KB diff read through a pipe is byte-identical to the same diff redirected to a file, with the final hunk line intact. The pre-fix revision returns 65536 bytes on that case, so the check is load-bearing rather than decorative.

All of the above is now committed rather than thrown away. detect-changed-connector-api-files.test.mjs holds 15 cases, one per bug that actually reached review on this branch: the three containment escapes, the _generator exclusion, the untracked-file blind spot, the comparison-base rule, the argument-parse rejections, the branch-diff fallback, and output size through a pipe. node:test and node:assert only — no dependencies, no runner config:

node --test .claude/skills/connector-api-changelog-entry/scripts/detect-changed-connector-api-files.test.mjs

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:

  • Whether /connector-api-changelog-entry <args> dispatches the skill as expected. Fallback is plain prose instructing the agent to use the skill.
  • Whether the agent's node-prefixed script invocation matches the allowlist prefix now that the skill pins it.
  • Whether the permission matcher accepts a bare repo-relative path in Edit(connector-api/changelog/README.md). If it does not match, the agent can read the diff but not write the entry.
  • Whether create-pull-request still pushes with no credential persisted in .git/config. It takes its own token input, 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_dispatch run before relying on Thursday's cron. continue-on-error means the regeneration pull request still lands either way.

Checklist

  • Documentation follows the contribution guidelines
  • Changelog accurately describes all changes – n/a, no reference or public documentation changed
  • All hyperlinks tested
  • SUMMARY.md updated if new pages added – n/a, no new pages

API

  • Changelog highlights the affected endpoints or operations – n/a, no API surface changed
  • Changelog highlights any deprecations – n/a
  • Deprecation Table updated if any deprecations – n/a

🤖 Generated with Claude Code

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>
@jnv
jnv requested a review from a team as a code owner August 7, 2026 12:17
@jnv jnv self-assigned this Aug 7, 2026
@moxly

moxly commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Review posted. View review · run

@moxly moxly 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.

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:

  1. In .github/workflows/regenerate-connector-api-reference.yml (line 94): the agent is granted unscoped Edit plus Bash on an in-repo script, so it can rewrite that script and then execute it — arbitrary shell in a job holding MOXLY_LITELLM_API_KEY and a persisted contents: write token. Scope Edit to Edit(connector-api/changelog/README.md) and Edit(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 the Bash grant entirely. Add persist-credentials: false to the checkout step and add-paths: connector-api/** to the create-pull-request step as defence in depth.
  2. 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 when steps.changelog.outcome != 'success'.
  3. In .github/workflows/regenerate-connector-api-reference.yml (line 85): consider guarding against a truncated diff on large regenerations — use --no-diff plus targeted reads, or tell the agent in the prompt to report truncation rather than drafting from a partial view.
  4. 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.

Comment thread .github/workflows/regenerate-connector-api-reference.yml Outdated
Comment thread .github/workflows/regenerate-connector-api-reference.yml Outdated
Comment thread .github/workflows/regenerate-connector-api-reference.yml Outdated
Comment thread .claude/skills/connector-api-changelog-entry/references/git-input-strategy.md Outdated
@moxly moxly added the risk: low label Aug 7, 2026
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>
@jnv
jnv requested a review from moxly August 7, 2026 14:07
@moxly

moxly commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Review posted. View review · run

@moxly moxly 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.

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 + allowlisted Bash prefix (was a warning) — Edit is now Edit(connector-api/changelog/README.md) and Edit(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: false on checkout (line 24) and add-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), and outcome rather than conclusion remains the right choice under continue-on-error.
  • Truncated diff on a large regeneration — solved properly rather than papered over: --path narrows the script's output, the two-step --no-diff → per-file flow is documented in SKILL.md:42 and git-input-strategy.md:22-27, and the prompt now instructs the agent to use it. The --path connector-api/operations/reservations.md example 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 ⚠️ sentence means the pull request still lands and says no entry was drafted — so it's a thing to watch on the dispatch run, not a blocker.

Fix All — prompt for AI agent

Fix the following issues in this PR:

  1. In .claude/skills/connector-api-changelog-entry/scripts/detect-changed-connector-api-files.sh (lines 41-44): the --path check is a lexical prefix test only, so --path connector-api/../.github passes it and then resolves outside connector-api/ once git normalizes the .. in the pathspec. Add a *..*) ... exit 1 ;; arm to the case in add_path before the connector-api/* arm. Also set set -f around the set -- $REQUESTED_PATHS split at line 91 (restoring it after) so a --path value cannot be glob-expanded. Both are needed for the comment at lines 85-86 and the claim at references/git-input-strategy.md:57 to be accurate as written.
  2. In .claude/skills/connector-api-changelog-entry/scripts/detect-changed-connector-api-files.sh (line 87): --path also 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's Priority: help text.
  3. In .github/workflows/regenerate-connector-api-reference.yml (line 24): no code change needed — add confirming that create-pull-request pushes successfully with no persisted credential to the list of things to check on the recommended workflow_dispatch run.

After all changes, run the project's linter/formatter.

Comment thread .github/workflows/regenerate-connector-api-reference.yml
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>
@jnv
jnv requested a review from moxly August 7, 2026 14:22
@moxly

moxly commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Review posted. View review · run

@moxly moxly 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.

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:

  • --path containment was lexical only (was a warning) — the *..*) arm at line 47 lands before the prefix test, and set -f / set +f (lines 104-106) bracket the split, so a value cannot be glob-expanded. Both halves of what I flagged are closed, and git-input-strategy.md:57 was updated in the same change so the reference doc states the actual rule rather than an aspiration.
  • --path narrowed the priority check too — fixed at the root instead of documented around: lines 87-95 probe all of connector-api/ for local changes before any narrowing, so successive --path calls can no longer answer from two different comparison bases. The Priority: help text (lines 31-33) and git-input-strategy.md:58 both say so. This is better than the note I suggested.
  • create-pull-request pushing with no persisted credential — now the fourth item in the PR description's live-run list, which is all this one asked for. The Edit(...) 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:

  1. In .claude/skills/connector-api-changelog-entry/scripts/detect-changed-connector-api-files.sh (lines 44-50): a --path value containing a newline passes the case validation as one string, then splits into two pathspecs at line 105 because IFS is a newline — so --path $'connector-api/operations\n.github/workflows' reaches outside connector-api/. Add a *"<newline>"*) arm rejecting newlines to the case in add_path, using the same literal-newline-in-quotes idiom as the IFS assignment at lines 102-103. Without it the comment at lines 97-99 and the claim at references/git-input-strategy.md:57 are still not true as written.
  2. In .claude/skills/connector-api-changelog-entry/scripts/detect-changed-connector-api-files.sh (line 115): optional — the { git diff; git diff; } | sort -u pipeline takes sort's exit status, so a git diff failure no longer aborts under set -e the way the previous redirect-to-temp-file did. Only worth changing if you want the --no-diff local path to fail as loudly as the arm below it.

After all changes, run the project's linter/formatter.

jnv and others added 2 commits August 7, 2026 20:55
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>
@jnv
jnv requested a review from moxly August 7, 2026 19:54
@moxly

moxly commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Review posted. View review · run

@moxly moxly 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.

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 --path value split into two pathspecs (was a warning) — gone. toScopedPathspec returns one string and execFileSync passes argv with no shell, so a value containing a newline is a single literal path that matches nothing. There is no IFS left to split on.
  • { git diff; git diff; } | sort -u swallowed git's exit status (was a nit) — gone. execFileSync throws on a non-zero git exit, so both the --no-diff and 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:

  1. 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.stdout is asynchronous when connected to a pipe — which is how the agent's Bash tool runs this — and process.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: the Stage regenerated reference step stages everything, so hasLocalChanges is always true in the workflow. Restructure the trailing if (hasLocalChanges) { … process.exit(0); } plus the following branch-diff block into a single if / else so the process ends naturally and Node flushes before exiting. Leave fail() and the --help path alone; their payloads are well under the pipe buffer.

After all changes, run the project's linter/formatter.

jnv and others added 2 commits August 7, 2026 22:26
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>
@jnv
jnv requested a review from moxly August 7, 2026 20:49
@moxly

moxly commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Review posted. View review · run

@moxly moxly 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.

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 through execFileSync with default stdio, so the child's stdout is a pipe (async on Linux); the reference is captured by handing the child an fd from openSync, 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. The piped.length > 64 * 1024 guard keeps the fixture honest if bigContents is 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/workflows and /etc exit 1 via toScopedPathspec's resolve-and-compare, and the embedded-newline value is asserted to produce no output — which is the correct post-execFileSync behaviour (one literal path matching nothing), not merely a rejection. git pathspec globbing still works inside the scope guards 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:

  1. 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", but fail() 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 the fail() exception, rather than changing the code.

After all changes, run the project's linter/formatter.

jnv and others added 2 commits August 7, 2026 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants