Repository navigation
feat: ADDON-90824 add RFCT CIM changes workflow job - #547
mbruzda-splunk wants to merge 8 commits into
Conversation
Opted-in add-ons can plan and run a Regression Field Comparison Tool comparison between the candidate package and a released baseline, so CIM mapping changes are documented from validated tool output instead of being reviewed by hand. plan-rfct applies event and changed-path routing, skips fork pull requests before any credential is used, and freezes the baseline and candidate revisions so a release published mid-run cannot change the comparison. run-rfct acquires both packages, pins the RFCT image by digest, and invokes a narrow composite action that holds no cloud or repository credentials. The runtime backend for provisioning two paired Splunk environments is still open, so both environment steps are stubs marked TODO: backend TBD and document the contract their replacement must satisfy. Preparation fails with backend-not-selected rather than reporting a comparison that did not happen. The job is advisory and is deliberately not wired into pre-publish while the publication policy is undecided. Co-Authored-By: Claude Code <noreply@anthropic.com>
A relative ./.github/actions path would resolve inside the add-on checkout and run add-on-controlled code, and a branch ref is mutable. Pin both references to the commit that introduced the actions so validation runs are reproducible. The release handoff replaces these with the released workflow tag. Co-Authored-By: Claude Code <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Dry-running the planner against splunk/test-addonfactory-repo surfaced two cases that would have failed the first comparison on the shared fixture. A release publishes the splunkbase package alongside partitioned deployment packages, so v0.8.7 carries both Splunk_TA_box-0.8.7.spl and Splunk_TA_box-0.8.7-_search_heads.spl. Requiring exactly one .spl asset rejected every such release. Identify the package by its version-suffixed name instead, keeping the single-asset fallback for unversioned naming. The fixture also carries a bare 1.0.0 tag with no assets, which outranks every release that does publish a package. Automatic baseline selection now falls through to the next older release instead of failing a run that never asked to be pinned there. An explicitly pinned baseline without a package stays an input failure. Co-Authored-By: Claude Code <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d92cc6fe67
ℹ️ 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".
| f"{quote(base, safe='')}...{quote(head, safe='')}" | ||
| "?per_page={per_page}&page={page}" | ||
| ) | ||
| files = paginate(url_template, token, lambda payload: payload.get("files") or []) |
There was a problem hiding this comment.
For comparisons touching more than 300 files, this pagination cannot retrieve the remainder: GitHub's compare endpoint documentation states that changed files appear only on the first page and are capped at 300 for the entire comparison. If a supported mapping path falls after that cap, paths can be empty and the planner incorrectly records no-mapping-changes; use an uncapped enumeration method or conservatively proceed whenever the response reaches the limit.
Useful? React with 👍 / 👎.
| commit = (payload.get("object") or {}).get("sha") | ||
| if not commit: | ||
| raise PlanInputError(f"Could not resolve a commit for tag {tag}") | ||
| return commit |
There was a problem hiding this comment.
When a release uses an annotated tag, the ref object's SHA is the tag-object SHA rather than the underlying commit SHA; GitHub's Git tag documentation shows that the commit is in the tag object's nested object.sha. Returning the ref SHA here records incorrect provenance and makes a manual release candidate's later actions/checkout attempt to check out a non-tree object, so inspect the ref object's type and recursively dereference tag objects to a commit.
Useful? React with 👍 / 👎.
|
|
||
| # The archive is untrusted data: list entry names only, never extract. | ||
| app_name_of() { | ||
| tar -tzf "$1" | head -n 1 | cut -d/ -f1 |
There was a problem hiding this comment.
For packages whose tar -t output exceeds the pipe buffer, head closes the pipe after the first entry and GNU tar exits with status 141; because this step enables set -euo pipefail, the baseline_app=$(app_name_of ...) or candidate assignment then aborts package verification even for a valid .spl archive. Read the first member without a short-circuiting pipeline, or explicitly tolerate tar's SIGPIPE while still validating actual archive errors.
Useful? React with 👍 / 👎.
| SECRET_RE = re.compile( | ||
| r"(?i)\b(pass(?:word|wd)?|token|secret|api[_-]?key|authorization)\b" | ||
| r"(\s*[:=]\s*|\s+)(\S+)" | ||
| ) |
There was a problem hiding this comment.
When RFCT stdout or stderr contains common forms such as Authorization: Bearer xyz, hec_token=xyz, access_token=xyz, or splunk_password=xyz, this regex leaves the credential exposed: it replaces only Bearer in the authorization example, and the word boundary prevents matching names joined with underscores. Because run_phase writes this sanitized text into uploaded log artifacts, an RFCT error that prints one of these standard credential forms leaks the temporary endpoint credential; redact the full authorization value and support credential-key prefixes and separators.
Useful? React with 👍 / 👎.
| status="${ACTION_STATUS:-failed}" | ||
| reason="" | ||
| if [ -z "${ACTION_STATUS:-}" ]; then | ||
| reason="${PREPARE_REASON:-execution-incomplete}" | ||
| fi |
There was a problem hiding this comment.
When image preflight, package acquisition, registry setup, or another step before preparation fails, both inputs here are empty and the manifest records only execution-incomplete; when the RFCT action itself emits ACTION_STATUS=failed, reason remains empty even though execution.json contains the specific failure reason. This defeats the documented machine-readable provenance for precisely the failures the artifact is intended to diagnose, so derive the reason from the failed phase or copy .reason from execution.json when it exists.
Useful? React with 👍 / 👎.
| if baseline_parsed["order"] >= candidate["order"]: | ||
| raise PlanInputError( | ||
| f"Baseline {baseline['tag']} must be older than candidate {candidate['tag']}" | ||
| ) |
There was a problem hiding this comment.
When a manual comparison explicitly selects prereleases with the same major, minor, and patch—for example baseline v2.0.0-beta.1 and candidate v2.0.0-beta.2—both are accepted by parsing and release lookup, but their order tuples are identical because the prerelease component was discarded. This check therefore rejects the genuinely older baseline as >= the candidate; either implement SemVer prerelease ordering or reject prerelease overrides consistently instead of accepting combinations that cannot be ordered.
Useful? React with 👍 / 👎.
The first fixture run failed with the planner error message from the commit before the package-selection fix: the workflow still pinned the actions to that older commit, so CI loaded superseded helper code while the workflow itself was current. Pinning to an immutable commit means every action edit needs a matching re-pin, which is easy to miss. Re-pin both references to the commit that last changed the action directories, and add a test that compares the pinned ref against that commit so a stale pin fails locally instead of in a fixture run. The test skips when git history is unavailable, as in a shallow checkout. Co-Authored-By: Claude Code <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2eb5e4ebe
ℹ️ 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".
| r"\A\d{12}\.dkr\.ecr\.[a-z0-9-]+\.amazonaws\.com/ta-automation/rfct" | ||
| r"@sha256:[0-9a-f]{64}\Z" |
There was a problem hiding this comment.
When the composite action is called directly, an image such as 123456789012.dkr.ecr.eu-west-1.amazonaws.com/ta-automation/rfct@sha256:... passes this regex even though the documented direct-call contract in README.md:1266 requires account 956110764581 in us-west-2. The comparison container is then given network access and a read-only mount of the configuration containing temporary endpoint credentials, so a mistaken or user-controlled override can run an unapproved image with those credentials; match the exact approved registry as the reusable-workflow wrapper already does.
Useful? React with 👍 / 👎.
| status, candidate_version = read_outcome(report_path) | ||
| execution["status"] = status | ||
| execution["candidate_version"] = candidate_version |
There was a problem hiding this comment.
When a compatible formatter exits zero but emits no Markdown—for example because it accepts the required flag but cannot render the report schema—this immediately derives a successful changes or no-changes status solely from comparison/report.json and never inspects the docs directory. The artifact can therefore be reported as successful without the promised versioned page (including the explicitly required no-change page); verify that the expected documentation output exists before setting the success status.
AGENTS.md reference: AGENTS.md:L3-L5
Useful? React with 👍 / 👎.
| token: ${{ github.token }} | ||
| event-name: ${{ github.event_name }} | ||
| base-branch: ${{ github.event_name == 'pull_request' && github.base_ref || github.ref_name }} | ||
| is-fork: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork || 'false' }} |
There was a problem hiding this comment.
For a same-repository PR in an add-on repository that is itself a fork, this evaluates to true because GitHub's repository-level isFork field identifies whether the repository itself is a fork, not whether this PR crosses repository boundaries. The planner consequently records fork-pull-request and skips a same-repository route that the public workflow contract says should run; compare head.repo.full_name with github.repository instead.
AGENTS.md reference: AGENTS.md:L3-L5
Useful? React with 👍 / 👎.
The fixture run reached package verification and failed with exit code 2 and "tar: stdout: write error". Piping tar -tzf into head closes the pipe after the first line, GNU tar reports a write error, and pipefail turns that into a step failure. It would have broken every add-on, not just the fixture. Capture the listing into a variable and slice it with parameter expansion, and send the package-count error to stderr so it is not swallowed by the command substitution that calls it. bsdtar exits 0 in the same situation, so a behavioural test cannot reproduce this off Linux. Guard it with a platform-independent check that the step never pipes an archive listing anywhere, and add a test that executes the step against built packages to cover app-identity resolution, checksums, mismatch, and an ambiguous package set. Co-Authored-By: Claude Code <noreply@anthropic.com>
E2E validation evidenceValidated against the shared fixture splunk/test-addonfactory-repo#392
Two real bugs found by running itBoth were invisible to the unit suite and to local dry runs. 1. Stale action pin — run 41490523870 2. Worth recording: bsdtar exits 0 in the same situation (verified up to a 282 KB listing), so bug 2 is plan-rfct — confirmed correctFrom artifact { "decision": "proceed", "reason": "mapping-change",
"changed_paths": ["package/default/props.conf"],
"baseline": { "tag": "v0.8.7", "asset_name": "Splunk_TA_box-0.8.7.spl",
"commit": "2290aa46709754336f43761075f2ac05f48edb91" },
"candidate": { "source": "build" },
"build_sha": "bdf0ba8986a3...", "pull_request_head_sha": "3ff14a40a6f2..." }Proven on live data: PR-to- run-rfct — reaches the expected blockerFinal run step outcomes:
This also exercised the evidence-on-failure contract for real. Uploaded { "schema_version": 1, "status": "failed", "reason": "execution-incomplete",
"app_code_name": "Splunk_TA_box",
"baseline": { "tag": "v0.8.7", "package_sha256": "73a58ed3c504..." },
"candidate": { "source": "build", "version": "0.392.41493003944",
"commit": "9134c5f0...", "package_sha256": "a7411964ad2b..." },
"splunk_version": "10.6.0", "sc4s_version": "3.40.0",
"changed_paths": ["package/default/props.conf"],
"rfct_image_digest": "", "phases": {}, "produced": ["result.json"] }App identity resolved from the real archive, both package checksums recorded, the synthetic PR version Not yet proven
Pre-existing, unrelated failures
CleanupFixture PR #392 and branch |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f5841ab0a
ℹ️ 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".
| options = baseline_candidates(releases, explicit, candidate) | ||
| for release, parsed in options: | ||
| described = describe_release(repository, token, release, parsed) | ||
| if explicit or not build_sha or described["commit"] != build_sha: |
There was a problem hiding this comment.
When a build candidate explicitly selects a baseline release whose tag resolves to build_sha—for example, a manual run from the commit currently tagged as that release—the explicit or branch accepts it, and the later equality check is only performed for release candidates. The workflow consequently compares two packages built from the same revision instead of rejecting or skipping the candidate revision as required by the public baseline-selection contract; apply the commit-equality check to explicit build baselines as well.
AGENTS.md reference: AGENTS.md:L3-L5
Useful? React with 👍 / 👎.
| """Create the evidence tree writable by the image's default UID 10001.""" | ||
| for relative in ("", "comparison", DOCS_RELATIVE, "logs"): | ||
| directory = output_directory / relative if relative else output_directory | ||
| directory.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
When a direct caller reuses an existing output directory, exist_ok=True preserves any prior comparison/report.json and generated docs. If the current compare or formatter invocation exits zero without replacing those files, read_outcome consumes the stale report and the action can report the previous run's changes or no-changes result rather than detecting missing current output; remove the known generated paths or require an empty output directory before execution.
AGENTS.md reference: AGENTS.md:L3-L5
Useful? React with 👍 / 👎.
| output_directory = Path(_input("OUTPUT_DIRECTORY") or "") | ||
| if not output_directory.is_absolute(): | ||
| raise ExecutionError("invalid-input", "output-directory must be an absolute path") | ||
| if output_directory.is_symlink(): | ||
| raise ExecutionError("invalid-input", "output-directory must not be a symbolic link") |
There was a problem hiding this comment.
When a direct caller places config-path underneath output-directory, both paths pass validation, but the documented upload of the returned artifact directory then includes the live RFCT configuration and its temporary endpoint credentials. The output bind also exposes the same file through a writable container path despite its separate read-only mount, so reject configurations contained by the output directory before execution.
AGENTS.md reference: AGENTS.md:L3-L5
Useful? React with 👍 / 👎.
Correction: a third pre-existing failure, and what it illustratesMy evidence comment listed only the two FOSSA checks as pre-existing. A fourth check, The fixture carries two documentation callers that both trigger on
Two runs therefore execute against the same commit and one always fails. The failing one logs So: a caller pinned to a mutable feature branch is still serving superseded, broken code long after Pre-existing fixture failures in full, none attributable to this change: |
Two tests added with the previous fixes passed locally and failed on CI. Both were test defects rather than product defects. The package verification test built baseline and candidate archives from identical content, which GNU tar renders byte for byte identically, so asserting the checksums differ compared a value with itself. Give each side a distinct payload. The pinned-action staleness test read git history that a shallow checkout does not have: git returned the pull-request merge commit instead of nothing, so the skip guard never fired and the comparison failed. Skip when the repository is shallow or the pinned commit is absent, and give this repository's own pre-commit job full history so the check actually runs there instead of silently skipping. Co-Authored-By: Claude Code <noreply@anthropic.com>
Fixture cleanup completePer
No caller reference needed restoring. The validation never modified fixture This repository's own CI is green on Reminder of what remains before feature acceptance, unchanged by this cleanup: the read-only ECR |
The default was the 1.0.0-beta.7 digest, whose formatter the compatibility preflight rejects, so an opted-in caller could not produce a conforming CIM changes result without overriding the input. RFCT 1.1.0 is published and its formatter accepts --ta-repo-path, confirming the compare and format flag names the action already relied on. Co-Authored-By: Claude Code <noreply@anthropic.com>
The image default change edited run_rfct.py, so the previous pin would have run the superseded helper. Co-Authored-By: Claude Code <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e49f9baed5
ℹ️ 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".
| raise PlanInputError("A pull request requires both a base SHA and a build SHA") | ||
| return base, head | ||
|
|
||
| after = _input("PUSH_AFTER") or _input("BUILD_SHA") |
There was a problem hiding this comment.
For an opted-in caller triggered when a release/* branch is deleted, GitHub still emits a push event, with after set to the all-zero SHA. This accepts that sentinel as the comparison head, so changed_paths requests compare/<before>...000..., receives a not-found response, and fails plan-rfct during routine branch cleanup instead of recording a skip; reject an absent after SHA or pass and check the payload's deleted field.
AGENTS.md reference: AGENTS.md:L3-L5
Useful? React with 👍 / 👎.
Description
Adds an opt-in RFCT (Regression Field Comparison Tool) CIM changes capability to the reusable
release workflow, so an add-on that changes its CIM mappings can compare the candidate package
against a released baseline and produce versioned CIM changes documentation as a run artifact.
Ticket: ADDON-90824.
Baseline:
developate884eeb2a88a13245dffc6aa67a5044064dc9a13.What this adds
plan-rfctjob +.github/actions/plan-rfct— event and changed-path routing, input validation,and immutable baseline/candidate selection. Skips fork PRs and
pull_request_targetbefore anycredential is used. Freezes the chosen revisions so a release published mid-run cannot change the
comparison. Emits
rfct-plan.json.run-rfctjob — acquires both packages, verifies app identity across them, records checksums,authenticates to ECR, resolves the image to a digest, checks formatter compatibility, invokes the
action, uploads evidence, and tears down even after failure.
.github/actions/run-rfct— a narrow composite action that validates prepared inputs and imagecapabilities, runs
compare reportthenformat-report docs-markdown, writesexecution.json,and cleans up its own containers. It receives no cloud or repository credentials, does not
provision environments, and does not fetch packages.
workflow_callinputs (rfct-enableddefaultfalse,rfct-image,rfct-baseline-release,rfct-candidate-release) and two optional secrets(
RFCT_ECR_AWS_ACCESS_KEY_ID,RFCT_ECR_AWS_SECRET_ACCESS_KEY).buildoutputspackage-versionandsource-sha, so RFCT never guesses the candidateversion from a package filename.
Existing callers are unaffected: with
rfct-enabledat itsfalsedefault, neither new job appearsin a run.
Open decision — this job cannot run yet
The runtime backend for provisioning two paired Splunk environments is unresolved (existing WFE
infrastructure, another supported remote backend, or runner-local Docker). The WFE path used by
run-knowledge-testssubmits an Argo workflow that runs the tests itself and returns results viaS3; it exposes no supported contract for two live Splunk endpoints held open across a comparison,
so it is not reusable as is.
Both environment steps are therefore stubs marked
TODO: backend TBD (ADDON-90824), eachdocumenting the contract its replacement must satisfy (inputs, installation and verification,
independent-parsing proof, credential masking, config output, lease expiry, idempotent teardown).
Preparation fails with
backend-not-selectedrather than reporting a comparison that did nothappen, and the action is gated on
prepared == 'true'. Everything that does not depend on thebackend is implemented and tested.
Deliberate decisions worth reviewing
run-rfctis not wired intopre-publish, because the publicationpolicy is still open. The README records how to add it for opted-in comparisons if blocking is
later agreed.
splunk/addonfactory-workflow-addon-release/.github/actions/...@6f80fae, not a relative./.github/actionspath, which would resolve inside the add-on checkout and executeadd-on-controlled code. The second commit pins the exact commit that introduced the actions. The
release handoff replaces this with the released workflow tag.
the app code name is derived from an untrusted package archive, so every value reaches a
run:body through
env:. A test asserts no${{appears in any RFCT step'srun:body.rfct-imagedefault is the verified beta.7 digest, whoseformatter predates the Table 1/Table 2 implementation and lacks
--ta-repo-path. Thecompatibility preflight runs before provisioning (and again inside the action, protecting a direct
caller) and rejects it, so beta.7 Markdown can never be uploaded as a conforming result. No
version is hardcoded as the first stable release; semantic-release determines it, and
v1.0.0and
v1.0.1already exist in RFCT history.absent, malformed, or outcome-less report is an
invalid-reportfailure.check-docs-changesleft alone. Its diff range is PR-shaped and is not a valid range forevery push or dispatch, as the design doc notes. Fixing it changes routing for every existing job
and every consumer, so it is not bundled here; RFCT is already insulated from it (
plan-rfcthasno dependency on it, and
run-rfctonly needsbuildwhen comparing a build candidate). Wortha separate ticket.
compare report/format-report docs-markdownfollow thedocumented contract but are unverifiable against beta.7; they are centralized in
compare_argv/format_argvwith a comment to re-confirm them against the first stable image.Security-relevant behavior
pull_request_targetskip before credentials are requested.before any add-on code, build hook, or sample could run.
persist-credentials: false; the candidate source is exported without.git.Docker socket, no home directory, no AWS or GitHub credentials enter the container.
redacted for credential-shaped values.
continue-on-erroris not used, so an execution failure cannot become a successful comparison.Checklist
README.mdhas been updated or is not requiredTesting done
Local required checks, on
d92cc6f:poetry run pytest— 126 passed, total coverage 91.65% (gate is 80%).New:
tests/test_plan_rfct.py(46),tests/test_run_rfct.py(53).pre-commit run --all-files— yamlfmt, actionlint, Ruff C901, and the workflowunused-input/secret hygiene check all pass.
Covered by unit tests: supported-path matching including additions, deletions and both sides of
renames; paginated compare and release walks plus the pagination bound; event-aware diff ranges
including a newly created branch with an absent
beforeand missing history; release ordering,draft/prerelease exclusion, and a baseline that is the built revision; app identity and
exactly-one-
.splenforcement; invalid and injection-shaped release references; fork skip assertedto make no API call; fixture-only deterministic rendering; no-change result; malformed JSON;
non-object and outcome-less reports; formatter preflight rejection with comparison proven not to
run; failed-phase evidence and exit-status retention; invocation cleanup on success and failure;
local-path, symlink,
.git, and digest-pinning validation; output mapping; and workflow-structureassertions for the required-job skip, digest-lookup guard, credential-removal ordering, upload and
teardown on failure, and
pre-publishremaining ungated.Not done — E2E validation is outstanding and is a blocker for feature acceptance. No
splunk/test-addonfactory-reporun is recorded, becauserun-rfctcannot complete until theruntime backend is selected. The remaining checklist boxes stay unticked for that reason. Once a
backend lands, the E2E evidence required by
AGENTS.md(workflow SHA, template caller branch andcommit, TA repo/head SHA/event/base branch/run URL, expected jobs, artifacts, cleanup) must be
recorded before release, along with a mapping-change PR, a docs-only skip, trusted
developandrelease-branch pushes, a fork skip, and a dispatch with explicit released versions. The template
caller in
addonfactory-repository-templatealso still needs the matching inputs, optionalsecrets, and dispatch fields.
🤖 Generated with Claude Code