Skip to content

Fix apps for SDK, build device matrix for CI, remove coreutils - #49

Merged
KenVanHoeylandt merged 3 commits into
mainfrom
develop
Sep 26, 2026
Merged

KenVanHoeylandt merged 3 commits into
mainfrom
develop

Conversation

@KenVanHoeylandt

@KenVanHoeylandt KenVanHoeylandt commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor
  • Fixes for SDK updates:
  • Fixed the file-selection handoff in EspNowBridge by providing valid launch options.
  • Removed coreutils project (it's built-into the OS now)
  • Updated build scripts to generate a matrix of devices
  • Moved all scripts into Scripts/ folder

@coderabbitai

coderabbitai Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: a6bd13e6-e350-4cff-af2e-089ed777af94

📥 Commits

Reviewing files that changed from the base of the PR and between 085c5db and 4c7ba44.

📒 Files selected for processing (11)
  • .github/workflows/main.yml
  • Apps/CoreUtils/date/CMakeLists.txt
  • Apps/CoreUtils/date/main/CMakeLists.txt
  • Apps/CoreUtils/date/main/Source/main.c
  • Apps/CoreUtils/echo/CMakeLists.txt
  • Apps/CoreUtils/echo/main/CMakeLists.txt
  • Apps/CoreUtils/echo/main/Source/main.c
  • Apps/CoreUtils/manifest.properties
  • Apps/EspNowBridge/main/Source/EspNowBridge.cpp
  • Tools/gh-generate-app-matrix.py
  • tactility.py
💤 Files with no reviewable changes (7)
  • Apps/CoreUtils/manifest.properties
  • Apps/CoreUtils/date/main/CMakeLists.txt
  • Apps/CoreUtils/date/CMakeLists.txt
  • Apps/CoreUtils/echo/main/Source/main.c
  • Apps/CoreUtils/echo/CMakeLists.txt
  • Apps/CoreUtils/echo/main/CMakeLists.txt
  • Apps/CoreUtils/date/main/Source/main.c

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The workflow now generates its build matrix from app directories with manifests. The CoreUtils Date and Echo apps and their package manifest are removed. EspNowBridge initializes two file-selection stream binding fields. The tactility.py script adds a Python 3 shebang and declares version 6.1.0.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 4c7ba

No actionable issue is established by the supplied evidence; the PR is mergeable after normal checks.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 4c7ba

Automatic app discovery expands the set of apps that CI builds and can publish. Directory names also flow into a build command without visible validation. Publication remains restricted to successful builds on main, but the new selection boundary merits review.

Retained concerns

  • Medium · security · inferred: A newly eligible app directory name can enter the build action's command text without name validation or shell isolation. If the external action executes that command through a shell, a crafted name could alter execution on the CI runner.
  • Medium · security · inferred: Manifest presence now determines build eligibility, and successful build artifacts enter the bundle published on main. The previous fixed app list no longer independently limits which newly added apps can reach publication.
Security review details

Security Blast Radius

  • inferred — The changed selection rule affects CI builds for repository-provided app directories and the app set eligible for publication after a successful main-branch run. A pull-request run does not directly enter PublishApps.

Security Findings and Attack Paths

  • inferred — A contributor able to add a manifest-bearing directory can place shell-significant characters in its name. That name flows unchanged into the build action's command input; command alteration depends on how the external action executes it.

Trust Boundaries and Controls

  • inferred — Manifest presence replaces the fixed matrix as the build-selection control. Successful artifacts can subsequently reach the credentialed publisher, but the app name is not itself passed to PublishApps and publication remains main-branch gated.

Resilience and Maintainability Implications

  • inferred — The Build dependency provides a normal-execution barrier before bundling, but the repository does not define artifact ownership across reruns or validate that each selected directory yields one distinct packaged app identity.

Hardening Proposals

  • proposed — Validate discovered names as single safe directory components and pass them to the build command as data rather than interpolating them into command text. Separately decide whether build discovery alone should authorize CDN publication.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
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.
Title check ✅ Passed The title accurately summarizes the main changes: SDK-related app fixes, CI matrix generation, and removal of CoreUtils. It is concise and specific.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@KenVanHoeylandt
KenVanHoeylandt merged commit d13ba5b into main Sep 26, 2026
23 checks passed
@KenVanHoeylandt
KenVanHoeylandt deleted the develop branch September 26, 2026 12:18
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