Skip to content

ci: emit and upload determinism signatemp: validate determinism signal-summary CI artifact flowl summary - #560

Closed
0xacabos wants to merge 1 commit into
Twigpine:mainfrom
0xacabos:temp/determinism-signal-validation
Closed

0xacabos wants to merge 1 commit into
Twigpine:mainfrom
0xacabos:temp/determinism-signal-validation

Conversation

@0xacabos

Copy link
Copy Markdown

Temporary validation PR for determinism signal-summary CI behavior.

Purpose:

  • verify success artifact upload
  • verify failure artifact upload
  • confirm signal-summary.json shape from CI

This PR is for validation only and will be reverted/closed after verification.

@kevincodex1 kevincodex1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks good to me

@Vasanthdev2004 Vasanthdev2004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed on head 58e7346 — CI red 🔴 (both Determinism jobs fail)

This PR adds a determinism-checks workflow, a signal summary builder, and a test file. The signalSummary.ts logic and tests look well-structured (manifest comparison, historical recurrence tracking, temporal classification). However, the CI workflow cannot run because the referenced npm scripts don't exist:

🔴 Both CI jobs fail:

  • Determinism job: error: Script not found "test:determinism" — the step bun run test:determinism fails immediately
  • Metadata Check job: error: Script not found "determinism:scan:json" — the step bun run determinism:scan:json fails immediately

The workflow also references bun run determinism:emit (in the "Emit normalized artifacts" steps), which would also fail if reached.

What's missing: The package.json scripts section needs entries for:

  • test:determinism → should run the test file in src/determinism/tests/serialization/
  • determinism:emit → should produce artifact manifests
  • determinism:scan:json → should produce scan reports

Other observations:

  • The PR description says "This PR is for validation only and will be reverted/closed after verification" — is this still the plan? If so, should it just be closed?
  • The workflow path trigger includes src/determinism/** and scripts/determinism/** — good scoping
  • Action SHAs verified: checkout@11bd719 ✅, setup-bun@4bc047a ✅, setup-node@49933ea ✅, upload-artifact@6546280 ✅
  • The signalSummary.ts logic is clean — manifest hashing, sorted-union comparison, recurrence stats with mode frequency
  • Tests cover the two main paths (no history vs. history available) and verify delta density calculation

Verdict: Needs changes — the workflow is non-functional without the package.json scripts. Also needs clarification on whether this is still a "temporary validation PR" per the description.

@gnanam1990 gnanam1990 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I can’t approve this yet. The signal-summary code/tests look fine, but the PR also adds a CI workflow that currently references determinism scripts that do not exist, so the workflow fails on its own setup. Please fix the workflow/script wiring first, then I’m happy to recheck.

@jatmn

jatmn commented May 13, 2026

Copy link
Copy Markdown
Collaborator

closing as stale

@jatmn jatmn closed this May 13, 2026
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.

5 participants