Skip to content

Implement Vi app - #50

Merged
KenVanHoeylandt merged 2 commits into
mainfrom
vi-app
Sep 27, 2026
Merged

KenVanHoeylandt merged 2 commits into
mainfrom
vi-app

Conversation

@KenVanHoeylandt

@KenVanHoeylandt KenVanHoeylandt commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor
  • Implement vi based on Busybox implementation
  • Updated tactility.py to 7.0.1
  • Update versions of all apps because of SDK changes

+ update versions of all apps because of SDK changes
@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.

📝 Walkthrough

Walkthrough

The pull request adds a BusyBox-based Vi app with build configuration, compatibility helpers, terminal key-reading code, app metadata, and documentation. It updates SDK tooling to use version-specific metadata and SDK-local sdkconfig files. It also increments the version name and version code in 19 existing app manifests.

Priority: ➖ Normal

Merge Risk: 🟡 Moderate · up to 70344

A failed SDK refresh can remove a previously usable cached SDK and block builds until it is downloaded again. Preserve the cache until its replacement is ready before merging.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 70344

The new editor and SDK migration warrant design review. A failed SDK replacement can remove a previously usable build cache, and a fatal editor error can leave the terminal in raw mode. No cross-app privilege expansion was established.

Retained concerns

  • Medium · reliability · observed: The SDK configuration migration deletes a legacy cached SDK before a replacement is available. Download failure or interruption can remove a previously usable build state, impairing build recovery and rollback.
  • Low · reliability · observed: Vi enters raw terminal mode before allocating its screen and file buffers. A fatal allocation failure exits without restoring terminal mode, extending the failure beyond the editor.
Security review details

Security Blast Radius

  • inferred — Observed compatibility-helper dependents remain inside Vi despite the helper range's high fanout; the SDK-cache failure instead affects builds for selected non-POSIX SDK platforms.

Trust Boundaries and Controls

  • observed — The editor checks ownership and write permissions before loading HOME/.exrc. Its command-line and environment inputs still operate through the new editor entrypoint; no broader authority or independent caller was established.

Resilience and Maintainability Implications

  • inferred — The SDK replacement ordering weakens recovery from a failed migration. Vi's fatal allocation path weakens containment of an editor failure to its terminal session; neither establishes a remote attack path on the available evidence.

Hardening Proposals

  • proposed — Stage and validate a replacement SDK before switching away from a legacy cache, preserving the old cache until the new build state is usable.
  • proposed — Ensure fatal exits after raw-mode entry restore terminal state, including when screen or file-buffer allocation fails.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 48 functions across 5 files. (24 skipped:… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely identifies the primary change: adding the Vi app. The SDK-related updates are secondary changes.
Full details: Docstring Coverage

Explanation

Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 48 functions across 5 files. (24 skipped: 24 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.

@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: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 7afdf957-66df-41ef-8cc6-15ec4269403c

📥 Commits

Reviewing files that changed from the base of the PR and between d13ba5b and 70344fd.

📒 Files selected for processing (30)
  • Apps/Brainfuck/manifest.properties
  • Apps/Breakout/manifest.properties
  • Apps/Calculator/manifest.properties
  • Apps/Diceware/manifest.properties
  • Apps/Doom/manifest.properties
  • Apps/EpubReader/manifest.properties
  • Apps/EspNowBridge/manifest.properties
  • Apps/GPIO/manifest.properties
  • Apps/GraphicsDemo/manifest.properties
  • Apps/HelloWorld/manifest.properties
  • Apps/M5UnitTest/manifest.properties
  • Apps/Magic8Ball/manifest.properties
  • Apps/MediaKeys/manifest.properties
  • Apps/MystifyDemo/manifest.properties
  • Apps/SerialConsole/manifest.properties
  • Apps/Snake/manifest.properties
  • Apps/TamaTac/manifest.properties
  • Apps/TodoList/manifest.properties
  • Apps/TwoEleven/manifest.properties
  • Apps/Vi/CMakeLists.txt
  • Apps/Vi/LICENSE
  • Apps/Vi/README.md
  • Apps/Vi/main/CMakeLists.txt
  • Apps/Vi/main/Include/libbb.h
  • Apps/Vi/main/Source/libbb.c
  • Apps/Vi/main/Source/main.c
  • Apps/Vi/main/Source/read_key.c
  • Apps/Vi/main/Source/vi.c
  • Apps/Vi/manifest.properties
  • tactility.py

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

Comment thread tactility.py
@KenVanHoeylandt
KenVanHoeylandt merged commit 3aa8df0 into main Sep 27, 2026
24 checks passed
@KenVanHoeylandt
KenVanHoeylandt deleted the vi-app branch September 27, 2026 21: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.

1 participant