Skip to content

feat(standards-sync): add targets filter and narrow the App token - #56

Merged
kyle-sexton merged 3 commits into
mainfrom
feat/standards-sync-target-filter
Jul 7, 2026
Merged

feat(standards-sync): add targets filter and narrow the App token#56
kyle-sexton merged 3 commits into
mainfrom
feat/standards-sync-target-filter

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Track B activation groundwork for the config-distribution engine (follows #50):

  • targets input — comma-separated exact-match allowlist of manifest repos (empty = all). This is the pilot / staged-rollout control: the first real sync runs against a single target without touching the manifest. Exact membership via yq set subtraction — no substring matching (github-iac alone matches nothing and fails the run). Spaces are stripped so a, b and a,b filter alike; a filter matching no target is a hard error (typo protection).
  • Plan log iterates the built matrix instead of the raw manifest, so the logged plan is exactly the target set the sync job will run, filter included.
  • App token narrowedpermission-contents: write + permission-pull-requests: write on the mint step, so the token carries only what the sync needs even if the installation ever has wider grants. Resolves the zizmor github-app finding (local zizmor v1.26.1 now clean on this file).
  • Header status note updated: the App secrets now exist on standards, and the SHA-pinned caller lands next.

Verification

Filter expression exercised locally with yq v4.53.3 against the live standards manifest: empty filter → all 4 targets; exact repo → 1; substring → 0 (errors); spaced pair → 2. actionlint + zizmor clean locally.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GbDWhcUtduCejgi7mcbMfy


Note

Medium Risk
Changes cross-repo sync scope and GitHub App token permissions; mistakes in the targets filter are guarded by hard errors, but a misconfigured allowlist could still limit or block intended rollouts.

Overview
Adds a targets workflow_call input so callers can run a staged rollout against a comma-separated, exact-match allowlist of manifest repos (empty = all). The plan job normalizes the filter, builds the matrix with yq set subtraction (no substring matches), and fails the run on junk filters or unknown repo names so typos cannot silently skip one repo while others sync.

The distribution plan log now walks the built matrix instead of every manifest target, so dry-run output matches what the sync job will execute when filtered.

On real syncs, actions/create-github-app-token now sets permission-contents: write and permission-pull-requests: write so minted tokens only carry branch/PR permissions even if the App installation is broader. Header comments are updated to reflect activation gating via caller + secrets rather than a draft “not active” note.

Reviewed by Cursor Bugbot for commit ad6a93e. Bugbot is set up for automated code reviews on this repo. Configure here.

Track B activation groundwork: a `targets` input narrows a run to an
exact-match allowlist of manifest repos (pilot + staged rollout), the
plan log iterates the built matrix so it always shows the filtered set,
and the minted App token is scoped to contents/pull-requests write only
(resolves the zizmor github-app finding). Header note updated now that
the App secrets exist and the standards caller is landing.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GbDWhcUtduCejgi7mcbMfy
@claude

claude Bot commented Jul 6, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather context and read changed files
  • Analyze diff for correctness, security, and CLAUDE.md alignment
  • Post review findings

Overall this is a clean, well-reasoned change. The filter logic is correct, the security posture improves, and the CLAUDE.md least-privilege principle is respected. A few notes below, ranging from one real concern to minor observations.

Filter logic — correct

The set-subtraction expression is sound:

select(strenv(only) == "" or ([.repo] - (strenv(only) | split(",")) | length) == 0)

[.repo] - allowlist is empty iff .repo is a member of allowlist — no substring matching, no accidental partial hits. The space-stripping (tr -d '[:space:]') happens before the value reaches yq, so "a, b" and "a,b" behave identically. The empty-result guard is good typo protection.

No shell-injection risk

$TARGETS flows through an env var → trstrenv(only) inside yq — it never touches an unquoted shell expansion or eval. This is the right pattern.


Finding 1 — Raw user input echoed into the workflow annotation (cosmetic, not a vulnerability)

standards-sync.yml L103

echo "::error::targets filter '$TARGETS' matches no manifest target"

$TARGETS is the raw, unvalidated input (spaces included, single-quoted but expanded by the shell). A value like foo::warning:: or a string containing newlines could produce a confusing or multi-line annotation. The risk is purely cosmetic — no injection path — but echoing $only (the normalized value) instead would be both safer-looking and more informative (it shows what actually reached yq):

echo "::error::targets filter '$only' matches no manifest target"

Fix this →


Finding 2 — Double-comma edge case produces a spurious empty-string entry (harmless)

"a,,b" | split(",")["a", "", "b"]. The empty string never matches a owner/name repo, so it has no practical effect — the guard still catches typos. Just worth knowing if the input description ever needs to document "no empty segments."


Finding 3 — permission-* inputs on create-github-app-token v3.2.0 will only be validated on the first live run

standards-sync.yml L163-L164

The PR description notes that zizmor and actionlint pass locally. However, permission-* inputs are a fine-grained feature; if the action version or the App installation doesn't support them, they would be silently ignored rather than erroring — meaning the token would carry broader grants than intended without any warning. This is inherently tested by the first real (non-dry-run) activation, but it may be worth adding a note in the workflow that the first real run should verify the minted token's scope via the GitHub API (https://api.github.com/installation/repositories or the token introspection endpoint) before it touches any target repo.


CLAUDE.md alignment

  • SHA-pinning: no existing pins were changed; no new uses: were introduced — compliant.
  • Least privilege: the permission-contents: write + permission-pull-requests: write narrowing is exactly the right direction and directly resolves the zizmor github-app finding.
  • No pull_request_target / workflow_run concerns — this is a workflow_call with no trigger changes.

Summary: Finding 1 is a low-severity cosmetic fix worth making; Findings 2 and 3 are observations only. The core logic — filter expression, empty-guard, token narrowing, plan-log sourcing from the filtered matrix — is correct and well-structured.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 27175d6d2c

ℹ️ About Codex in GitHub

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

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

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

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

Comment thread .github/workflows/standards-sync.yml Outdated
cursor[bot]
cursor Bot approved these changes Jul 6, 2026
@claude

claude Bot commented Jul 6, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather context and read changed files
  • Analyze diff for correctness, security, and CLAUDE.md alignment
  • Post review findings

This is a clean, well-reasoned two-commit change. The filter logic is sound, the Codex P2 finding is fully addressed, and the security posture improves. A few observations below.

Filter logic — correct

The yq set-subtraction expression is the right approach:

select(strenv(only) == "" or ([.repo] - (strenv(only) | split(",")) | length) == 0)

[.repo] - allowlist is empty iff .repo is a member of the allowlist — exact matching, no substring hits. Space stripping via tr -d '[:space:]' happens before only ever reaches yq, so "a, b" and "a,b" are equivalent.

Codex P2 fully addressed by commit 08f5bd2

The fix commit adds the set-difference check against the whole manifest, not just an empty-result guard:

unmatched="$(only="$only" yq -r '((strenv(only) | split(",")) - [.targets[].repo]) | map(select(. != "")) | join(",")' "$MANIFEST")"

(filter entries) − (all manifest repos) catches any entry that doesn't exist in the manifest — the map(select(. != "")) correctly strips empty segments from degenerate input like "a,,b". A partial mismatch (org/real-repo,org/typo) now fails before any sync job runs.

Error message uses normalized output, not raw user input

echo "::error::targets filter names unknown manifest targets: $unmatched"

$unmatched is yq-computed (manifest-derived repo names), not $TARGETS. This is preferable — it shows exactly which entries were unrecognized, not whatever the caller typed.

Plan log now reflects the filtered set — correct

standards-sync.yml L136

done < <(yq -p=json -r '.include[].repo' <<<"$MATRIX")

Sourcing from the built matrix (post-filter) instead of yq -r '.targets[].repo' "$MANIFEST" means the logged plan is an exact match to what the sync job will execute. The inner layout/file lookups still go to the manifest by repo name, which is correct — those are stable lookups against first-party data.

App token narrowing — correct direction

standards-sync.yml L169-L170

permission-contents: write
permission-pull-requests: write

This resolves the zizmor github-app finding and directly satisfies the CLAUDE.md least-privilege principle. One note: permission-* inputs on create-github-app-token@v3.2.0 are a fine-grained feature; if the App installation or action version doesn't honour them, they'd be silently ignored rather than erroring. The first real (non-dry-run) activation should verify the minted token's actual scope — the https://api.github.com/installation/repositories endpoint or token introspection will confirm.

No shell injection

$TARGETS flows: env vartr -d '[:space:]'onlystrenv(only) inside yq. It never touches an unquoted shell expansion. Annotation-injection risk is also minimal — $unmatched contains manifest-derived owner/name strings; a ::warning:: sequence mid-line is parsed as message content, not a new annotation command.

CLAUDE.md alignment

  • SHA-pinning: no existing pins changed, no new uses: introduced — compliant.
  • Least privilege: token narrowing is in the right direction.
  • No pull_request_target / workflow_run trigger changes — not a concern.
  • No secrets echoed — clean.

Summary: The logic is correct, the Codex P2 partial-mismatch concern is properly resolved, and the token-scope improvement lands exactly as described. No blocking findings — ready to merge once the first live activation verifies the permission-* inputs take effect on the minted token.

A typo in one of several filter entries silently dropped that repo from
a staged rollout while the rest synced. Validate the full allowlist
against the manifest and fail naming the unknown entries (Codex review).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GbDWhcUtduCejgi7mcbMfy

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 08f5bd2. Configure here.

Comment thread .github/workflows/standards-sync.yml

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale comment

Risk: medium. Cursor Bugbot reported 1 unresolved finding (comma-only targets bypass validation) and the Bugbot check finished as skipped, so I am not approving. Human review is needed; no reviewers were assigned because no suitable non-author reviewers are available in this repository.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather context and read changed files
  • Analyze diff for correctness, security, and CLAUDE.md alignment
  • Post review findings

This is a clean, complete implementation across three commits. The two prior reviews covered the original feature commit and the Codex P2 fix; the third commit (ad6a93e) adds the all-comma/all-space guard that Cursor Bugbot flagged. All three together are correct. Notes below, in order of severity.

Normalization pipeline is correct

standards-sync.yml L98

only="$(tr -d '[:space:]' <<<"$TARGETS" | tr -s ',' | sed 's/^,//; s/,$//')"

The three-stage pipe handles every degenerate input correctly: tr -s ',' squeezes consecutive commas (so "a,,b""a,b" before the yq split ever runs), then sed strips any residual leading/trailing comma. This means the only variable is already clean when it reaches both the matrix-build and unmatched-check yq calls — the absence of map(select(. != "")) in the yq expression is not a gap.

All-comma/all-space guard (commit ad6a93e) is correct

standards-sync.yml L99-L102

if [ -n "$(tr -d '[:space:]' <<<"$TARGETS")" ] && [ -z "$only" ]; then
  echo "::error::targets filter '$TARGETS' contains no repo names"
  exit 1
fi

The left-hand condition checks that $TARGETS contained something other than pure whitespace; the right-hand condition checks that normalization reduced it to nothing. This correctly gates inputs like ",", " , ", ",, ,". Tracing , ,tr -d '[:space:]'",,"tr -s ','","sed"" → guard fires. ✓

Minor observation only: the error echoes the raw $TARGETS, not the normalized $only (which would be empty anyway and unhelpful here). $TARGETS is the right thing to show in this path since the point is "here's what you typed that we can't parse." No change needed — this is just noting intent is clear.

Unmatched-entry check correctly catches partial mismatches

standards-sync.yml L111-L117

unmatched="$(only="$only" yq -r '((strenv(only) | split(",")) - [.targets[].repo]) | join(",")' "$MANIFEST")"

(filter entries) − (manifest repos) = filter entries with no manifest counterpart. A single-typo entry like org/typo-repo in a multi-target filter will be caught and reported before any sync job runs. $unmatched is manifest-derived owner/name strings — no annotation-injection risk.

Plan log sourcing is consistent

standards-sync.yml L142

done < <(yq -p=json -r '.include[].repo' <<<"$MATRIX")

-p=json is required here since $MATRIX is a JSON string, not a YAML file. The plan now exactly mirrors the sync job's target set — filter included.

Token narrowing — correct direction, verify on first live run

standards-sync.yml L175-L176

permission-contents: write
permission-pull-requests: write

Directly addresses the zizmor github-app finding. One standing note from both prior reviews: permission-* inputs on create-github-app-token@v3.2.0 are fine-grained tokens; confirm the minted token's actual scope via https://api.github.com/installation/repositories or token introspection on the first live (non-dry-run) activation, since unsupported inputs would be silently ignored rather than erroring.

CLAUDE.md alignment

  • SHA-pinning: no existing pins changed, no new uses: entries — compliant.
  • Least privilege: token narrowing satisfies CLAUDE.md's principle.
  • No pull_request_target/workflow_run trigger changes — not a concern.
  • No secrets echoed — clean.

Summary: All three commits are correct, the Codex P2 partial-mismatch finding and the Cursor comma-only bypass finding are both resolved, and the security posture improves. No blocking findings — ready to merge once the first live activation verifies the permission-* inputs take effect on the minted token.

Normalize the allowlist (squeeze/trim commas after space-stripping) so
split never yields empty tokens, and hard-error when a non-empty filter
normalizes to nothing (e.g. ",,") instead of green-planning zero targets
(Cursor review).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GbDWhcUtduCejgi7mcbMfy

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Risk: medium. Cursor Bugbot passed on the latest commit and its prior finding is resolved; approving. No reviewers assigned.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@kyle-sexton
kyle-sexton merged commit 9cf6d13 into main Jul 7, 2026
32 checks passed
@kyle-sexton
kyle-sexton deleted the feat/standards-sync-target-filter branch July 7, 2026 01:19
kyle-sexton added a commit that referenced this pull request Jul 16, 2026
#109)

## Summary

Closes decision #56 (Decisions Log:
https://claude.ai/code/artifact/232ecdce-8316-4880-8c0a-dc3c7dcf3a63).

**Corrected premise:** the decision assumed 3 rolling-maintenance-issue
workflows, each with a different dedup mechanism. Empirically there are
**4**: `link-check.yml`, `queue-monitor-liveness.yml`, and
`tool-version-drift-check.yml` all used the *same* title-search dedup
(`gh issue list --search 'in:title "..."'`), while only
`pulumi-version-drift-check.yml` already used an embedded-HTML-marker.
The intent (converge on the marker mechanism) still holds — it's a
3-file port to the 4th file's existing pattern, not "each different."

## Why markers over title search

Title search breaks on a retitled issue (the search string no longer
matches) and can false-positive-match an unrelated issue that happens to
share title text. A marker embedded in the issue body is exact and
survives a retitle — the same reasoning `pulumi-version-drift-check.yml`
already documents for its own mechanism.

## Implementation

Ported all 3 to marker-based lookup, inlined per file (not a shared
composite action). I could not confirm from GitHub's docs, and found no
existing precedent in this repo, that a reusable workflow's `uses:
./.github/actions/...` reference resolves against its own repo when
called cross-repo — rather than build on unverified behavior for
something that gates real issue-tracking, I inlined the lookup logic
directly in each file, matching `pulumi-version-drift-check.yml`'s own
single-script style.

- `link-check.yml`: marker prepended to lychee's generated report file
before it's passed to `create-issue-from-file`.
- `queue-monitor-liveness.yml`, `tool-version-drift-check.yml`: marker
added as the first line of their existing hand-built issue-body
heredocs.

Each workflow's own marker is scoped to it (`<!--
ci-workflows:<workflow-name>:v1:active -->`), consistent with
`pulumi-version-drift-check.yml`'s naming.

Search is scoped `state=open`, matching the original title-search's
scoping. `pulumi-version-drift-check.yml` can safely search `state=all`
only because it swaps its marker to a `:resolved` sentinel on close;
none of these 3 do that, so `state=all` would keep matching a closed
issue's stale `:active` marker on every later run.

## Verification

- `zizmor` on all 3 modified files: no findings (3 suppressed, matching
repo baseline).
- `actionlint`: clean on `link-check.yml` and
`queue-monitor-liveness.yml`. `tool-version-drift-check.yml` hangs
locally in this environment — confirmed **pre-existing**, reproduces
identically against the unmodified file on `main`, unrelated to this
change. Repo's own hosted CI actionlint will validate it on this PR.
- No behavior change to what each workflow actually checks — only the
tracking-issue lookup mechanism changed.

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant