Skip to content

Conversation

@rowan-stein
Copy link
Contributor

$Replaces map/object yq comparisons with scalar checks; ensures deterministic file permissions on Linux; adds minimal debug.\n\nCloses #18

@rowan-stein
Copy link
Contributor Author

$Requesting review.

Summary of changes in this PR:

  • CI live test: treat CLI output as YAML; write download.yaml and parse with yq -p=yaml
  • Avoid yq map comparisons: split scalar checks (type int, then > 0)
  • Preserve Linux-only permissions enforcement (umask 077; expect 0600)
  • Keep token-gated live tests to avoid failures on forks

CI is green. Please focus on:

  • YAML/JSON parsing choices and portability across yq v4.44.x
  • Scalar comparisons and stability of checks
  • Linux gating for permission checks and overall test determinism
  • Any improvements to clarity of workflow logs.

Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

Thanks for the focused fixes — the YAML handling, scalar yq checks, token-gated live test, and clearer logs look solid overall.

Blocking change request:

  • Linux mode check currently runs chmod 600 "$ABS" before reading permissions. This masks whether the CLI respected umask 077 during file creation. Please remove the chmod and assert the mode of the freshly created file directly (expect 0600). This ensures deterministic enforcement and will catch regressions.

Recommended tweaks (non-blocking, for determinism/clarity):

  • Use yq -p=yaml consistently when extracting ID from OUT_SEARCH, and add an explicit pre-check that search returned results, e.g. yq -p=yaml -e ".data | type == \"!!seq\" and (.data | length) > 0" before reading .data[0].id. This avoids brittle failures if the query ever yields 0 hits.
  • In the Rust toolchain step, consider specifying with: components: clippy, rustfmt to guarantee those components are available. CI is green, but explicitly requesting components improves reliability.
  • Optional: the hexdump currently targets download.yaml. If you want a quick content sanity check of the downloaded asset itself, you could hexdump the first bytes of $ABS or print file -b "$ABS" (JPEG/PNG), though the current existence/size checks are sufficient.

Once the permission check is corrected, I expect this to be good to approve.

…sions and YAML parsing; request rustfmt/clippy components (Issue #18, PR #19)}
Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

Looks great after the updates. The Linux permission assertion now validates the mode directly; YAML parsing is deterministic with -p=yaml and a pre-check for non-empty results; scalar-only yq checks are in place; rustfmt/clippy components are explicitly requested; and logs are concise with helpful context.

Approving. ✅

@rowan-stein rowan-stein merged commit b826c5b into main Oct 20, 2025
2 checks passed
@rowan-stein rowan-stein deleted the fix/issue-18-ci-download-test branch October 20, 2025 22:46
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.

3 participants