Skip to content

fix: validate token shape before treating post-run config as rotated#31

Merged
michieldegezelle merged 1 commit into
mainfrom
fix-token-writeback-validation
Jul 23, 2026
Merged

fix: validate token shape before treating post-run config as rotated#31
michieldegezelle merged 1 commit into
mainfrom
fix-token-writeback-validation

Conversation

@michieldegezelle

Copy link
Copy Markdown
Collaborator

Summary

run_sampler.yml's "Capture partner token after the run and write back if rotated" step reads AFTER_TOKEN via jq -r and compares it directly to BEFORE_TOKEN, without checking it's actually a non-empty string first — unlike the earlier "Load PARTNER_CONFIG_JSON" step, which does validate shape before proceeding.

If config.json ends up missing partnerCredentials[partner].token after the run (e.g. a partial/crashed write by the CLI), jq -r prints the literal string "null", which differs from BEFORE_TOKEN and reads as "rotated" — triggering an unconditional gh secret set that overwrites the good secret with a broken config. Since PARTNER_CONFIG_JSON_<partner> has no other writer, that's an unrecoverable regression for that partner until someone manually re-authorizes it.

Fix: validate the post-run token's shape the same way the load step already does, and fail closed (refuse to write back) rather than silently corrupting the secret.

How this was found

Flagged by CodeRabbit while nl_market and lu_market temporarily inlined this job into their own wrapper workflows (unrelated experiment - installing silverfin-cli from a branch instead of main to gather feedback on a different change). This reusable workflow is the actual upstream source both copies inherited the bug from - already fixed in both:

Test plan

  • YAML validated (ruby -ryaml, actionlint - one pre-existing queue: max false positive unrelated to this change)
  • No CI in this repo to run automatically; safe to review directly, minimal 4-line diff

🤖 Generated with Claude Code

AFTER_TOKEN was compared to BEFORE_TOKEN without checking it's actually a
non-empty string first, unlike the "Load PARTNER_CONFIG_JSON" step, which does
validate shape. If config.json ends up missing partnerCredentials[partner].token
after the run (e.g. a partial/crashed write by the CLI), `jq -r` prints the
literal string "null", which differs from BEFORE_TOKEN and reads as "rotated" -
triggering an unconditional gh secret set that overwrites the good secret with
a broken config. Since this secret has no other writer, that's unrecoverable
for the partner until someone manually re-authorizes. Fail closed instead.

Found via CodeRabbit review on lu_market#765 and nl_market#893, which had
temporarily inlined this job (unrelated experiment); this is the actual
upstream source both copies inherited the bug from.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

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

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

Tip: disable this comment in your organization's Code Review settings.

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d253b379-f894-4c88-9d52-b0a313f831e0

📥 Commits

Reviewing files that changed from the base of the PR and between 55ee8aa and 2431342.

📒 Files selected for processing (1)
  • .github/workflows/run_sampler.yml

Walkthrough

The sampler workflow now validates the post-run partner token before comparing it with the previous value or writing rotated credentials back to the partner secret.

Changes

Partner token write-back safety

Layer / File(s) Summary
Post-run token validation
.github/workflows/run_sampler.yml
The workflow uses jq to require a present, non-empty string token in config.json, failing before any potential secret write-back when validation fails.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: validating the post-run token shape before write-back.
Description check ✅ Passed The description covers the change and test plan, though it omits the template's Fixes #, type-of-change, and checklist sections.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-token-writeback-validation

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 Checkov (3.3.8)
.github/workflows/run_sampler.yml

Traceback (most recent call last):
File "/usr/local/bin/checkov", line 2, in
from checkov.main import Checkov
ModuleNotFoundError: No module named 'checkov'


Comment @coderabbitai help to get the list of available commands.

@michieldegezelle

Copy link
Copy Markdown
Collaborator Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@michieldegezelle
michieldegezelle merged commit 53e0ebd into main Jul 23, 2026
1 check passed
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