Skip to content

app-module and TactilitySDK improvements - #665

Merged
KenVanHoeylandt merged 9 commits into
mainfrom
develop
Sep 27, 2026
Merged

KenVanHoeylandt merged 9 commits into
mainfrom
develop

Conversation

@KenVanHoeylandt

@KenVanHoeylandt KenVanHoeylandt commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

New features:

  • SDK version 7.0.0 packages include version metadata and platform-specific application settings.
  • Apps gain POSIX support for file polling, terminal attributes, and standard input/output, plus additional C and POSIX functions.
  • Shell commands can launch installed headless apps using their registered app settings.
  • Apps can exit with a status code, and terminal clear commands support clearing from the cursor to the end, from the start through the cursor, or the entire display.

Fixes:

  • SDK integration builds locate the build tool reliably and use settings for the selected platform.
  • ESP32 file operations handle invalid paths and existing directories more consistently.

P4 devices are reaching the 4MB app partition limit. This will give them
more space.
@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The SDK release process now packages target-specific application configuration and version metadata. The build tool uses metadata and configuration associated with the selected SDK version. The app module adds readiness polling and platform-specific POSIX I/O wrappers, with integration tests for stdin metadata, polling, and app exit. The shell can run eligible installed headless app binaries as commands. The terminal emulator now handles CSI J erase modes 0, 1, and 2.

Priority: ➖ Normal

Merge Risk: 🟡 Moderate · up to 728a6

This change adds a way for an app to call exit() and end only itself, plus new shell and SDK tooling behavior. Several problems should be fixed before merging. An app's exit status may not reach its parent correctly. Exiting while C++ objects are still alive is undefined behavior. POSIX SDK builds can fail because a configuration file they do not need is required. Calling fstat with a null buffer on an app descriptor can crash. Two installed apps with the same binary name produce the same shell command, so one of them cannot be run by name.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 728a6

Installed apps gain a new command-name launch path, and app termination now uses a jump that may not preserve C++ cleanup or reliably report exit status. Built-in shell commands retain priority, and normal app exit still reaches scheduler cleanup, but these changes merit design review.

Retained concerns

  • Medium · security · inferred: Installed binaries enter a shared shell command namespace by basename. Two packages can supply the same name, and first-match dispatch may execute a different installed app than the caller intended. If an untrusted package can be registered, this also creates a conditional command-hijacking path; built-in commands retain priority.
  • Medium · reliability · inferred: The new app-exit jump can bypass destructors in C++ app or loader frames. It also reads an exit status changed in a non-volatile local object after setjmp. These conditions could undermine cleanup or result reporting when an app calls exit; the inspected evidence does not establish a concrete destructor-bearing call stack or a security-dependent consumer of the status.
Security review details

Security Blast Radius

  • inferred — A colliding installed binary name can affect unqualified invocations of that installed command, including callers that pass arguments to it. Built-in commands are enumerated first and are not displaced by an installed-name collision in this path.

Security Findings and Attack Paths

  • inferred — If an actor can register an installed headless package with a colliding binary basename, first-match shell lookup may launch that package when another installed command was intended. The evidence does not establish package-install authority or an actual exploit.

Trust Boundaries and Controls

  • observed — Shell discovery requires a registered headless manifest and gives memory-backed built-ins precedence. Manifest registration rejects duplicate IDs, but the shell's installed-command lookup compares derived names, not those IDs.

Resilience and Maintainability Implications

  • inferred — The owning-task teardown limits the normal exit path, but a jump across live C++ resource owners could defeat cleanup before that teardown begins. The inspected test exercises exit from an app entry point, not this conditional resource-owning path.

Hardening Proposals

  • proposed — Define collision handling for installed command names and the intended exit contract for app-created threads; use an exit transition that preserves required C++ cleanup and stable result reporting.
🚥 Pre-merge checks | ✅ 3 | ❌ 1 | ❓ 1

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 10.96% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 146 functions across 30 files. (1 skipped… Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title identifies two affected areas, but “improvements” is too vague to describe the primary changes, which include app I/O wrapper refactoring and TactilitySDK packaging updates. Use a more specific title, such as “Refactor app I/O wrappers and update TactilitySDK packaging.”
✅ Passed checks (3 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.
Full details: Docstring Coverage

Explanation

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

✨ 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.

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

Actionable comments posted: 3


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 0fae3099-a03e-4e80-8deb-d7c65a4d660e

📥 Commits

Reviewing files that changed from the base of the PR and between 0580eb6 and 75732e1.

⛔ Files ignored due to path filters (5)
  • partitions-16mb-no-sd-dev.csv is excluded by !**/*.csv
  • partitions-16mb-no-sd.csv is excluded by !**/*.csv
  • partitions-16mb-with-sd.csv is excluded by !**/*.csv
  • partitions-32mb-no-sd-dev.csv is excluded by !**/*.csv
  • partitions-32mb-no-sd.csv is excluded by !**/*.csv
📒 Files selected for processing (31)
  • .github/actions/build-sdk-posix/action.yml
  • .github/actions/build-sdk/action.yml
  • Buildscripts/CDN/upload-sdk-files.py
  • Buildscripts/TactilitySDK/sdkconfig.app.esp32
  • Buildscripts/TactilitySDK/sdkconfig.app.esp32c6
  • Buildscripts/TactilitySDK/sdkconfig.app.esp32p4
  • Buildscripts/TactilitySDK/sdkconfig.app.esp32s3
  • Buildscripts/release-sdk-esp32.py
  • CMakeLists.txt
  • Documentation/ideas.md
  • Modules/app-module/CMakeLists.txt
  • Modules/app-module/include/app/io.h
  • Modules/app-module/private/app/private/stdio_wrap.h
  • Modules/app-module/private/app/private/stdio_wrap_posix.h
  • Modules/app-module/source/io.cpp
  • Modules/app-module/source/module.cpp
  • Modules/app-module/source/stdio_wrap.cpp
  • Modules/app-module/source/stdio_wrap_apple.cpp
  • Modules/app-module/source/stdio_wrap_elf.cpp
  • Modules/app-module/source/stdio_wrap_esp32.cpp
  • Modules/app-module/source/stdio_wrap_posix.cpp
  • Modules/app-module/tests/CMakeLists.txt
  • Modules/app-module/tests/source/io_test.cpp
  • Modules/app-posix-module/tests/CMakeLists.txt
  • Modules/c-symbols-module/source/module.cpp
  • Modules/posix-symbols-module/source/module.cpp
  • Tactility/Source/app/shell/LineEditor.cpp
  • Tactility/Source/app/terminal/vterm/vterm.c
  • device.py
  • tactility.py
  • tactility.py.json

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

Comment thread Buildscripts/CDN/upload-sdk-files.py Outdated
Comment thread Modules/app-module/source/stdio_wrap.cpp Outdated
Comment thread tactility.py

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

Actionable comments posted: 3


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 1110ab61-7604-471d-84cd-256817cfc9e0

📥 Commits

Reviewing files that changed from the base of the PR and between 2ffe072 and e10566f.

📒 Files selected for processing (12)
  • Buildscripts/CDN/upload-sdk-files.py
  • CMakeLists.txt
  • Modules/c-symbols-module/source/module.cpp
  • Modules/posix-symbols-module/source/module.cpp
  • Platforms/platform-esp32/source/mkdir.cpp
  • Platforms/platform-esp32/source/root_dir.cpp
  • Platforms/platform-esp32/source/unistd.cpp
  • Platforms/platform-esp32/source/vfs_null_path.cpp
  • Tactility/Private/Tactility/app/shell/Shell.h
  • Tactility/Source/app/shell/Shell.cpp
  • Tactility/Tests/Source/ShellCompletionTest.cpp
  • tactility.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • Buildscripts/CDN/upload-sdk-files.py

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

Comment thread Modules/c-symbols-module/source/module.cpp
Comment thread Tactility/Source/app/shell/Shell.cpp
Comment thread Tactility/Source/app/shell/Shell.cpp Outdated

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

Actionable comments posted: 3


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 222b3521-094f-4697-8977-d92443454316

📥 Commits

Reviewing files that changed from the base of the PR and between e10566f and acbb8b2.

📒 Files selected for processing (13)
  • Modules/app-module/include/app/io.h
  • Modules/app-module/private/app/private/stdio_wrap.h
  • Modules/app-module/source/io.cpp
  • Modules/app-module/source/stdio_wrap.cpp
  • Modules/app-module/source/stdio_wrap_esp32.cpp
  • Modules/app-module/source/stdio_wrap_posix.cpp
  • Modules/app-module/tests/source/io_test.cpp
  • Modules/c-symbols-module/source/module.cpp
  • Tactility/Private/Tactility/app/shell/Run.h
  • Tactility/Private/Tactility/app/shell/Shell.h
  • Tactility/Source/app/shell/Run.cpp
  • Tactility/Source/app/shell/Shell.cpp
  • tactility.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • Modules/c-symbols-module/source/module.cpp
  • Tactility/Private/Tactility/app/shell/Shell.h
  • Modules/app-module/include/app/io.h

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

Comment thread Modules/app-module/source/stdio_wrap.cpp
Comment thread tactility.py
Comment thread Tactility/Source/app/shell/Run.cpp

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

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 865d3473-79d9-4727-9301-102522d3a84f

📥 Commits

Reviewing files that changed from the base of the PR and between acbb8b2 and 728a6aa.

📒 Files selected for processing (9)
  • CMakeLists.txt
  • Modules/app-module/include/app/scheduler.h
  • Modules/app-module/private/app/private/stdio_wrap_posix.h
  • Modules/app-module/source/scheduler.cpp
  • Modules/app-module/source/stdio_wrap_apple.cpp
  • Modules/app-module/source/stdio_wrap_elf.cpp
  • Modules/app-module/source/stdio_wrap_esp32.cpp
  • Modules/app-module/source/stdio_wrap_posix.cpp
  • Modules/app-module/tests/source/io_test.cpp

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

Comment thread Modules/app-module/source/scheduler.cpp Outdated
Comment thread Modules/app-module/source/scheduler.cpp Outdated
@KenVanHoeylandt KenVanHoeylandt changed the title Develop app-module and TactilitySDK improvements Sep 27, 2026
@KenVanHoeylandt
KenVanHoeylandt merged commit cba73d1 into main Sep 27, 2026
65 checks passed
@KenVanHoeylandt
KenVanHoeylandt deleted the develop branch September 27, 2026 20:30
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