Skip to content

Refactor/extract file replace - #21

Merged
vaulttec-dev merged 33 commits into
developfrom
refactor/extract-file-replace
Oct 16, 2026
Merged

vaulttec-dev merged 33 commits into
developfrom
refactor/extract-file-replace

Conversation

@vaulttec-dev

@vaulttec-dev vaulttec-dev commented Oct 16, 2026 •

Copy link
Copy Markdown
Owner

User description


Summary by cubic

Adds the Makerdiary nRF52840 dongle as the primary development board, with FIDO2 passkeys, tap-based pam_u2f login, and Git commit signing; the ESP32-C6 remains a test board. The nRF bootloader accepts only signed firmware and does not expose flash, but the board is not yet suitable for real secrets.

  • Backups include passkeys in sealed sectors; restores support --only for named items.
  • The CLI adds passkey management, commit and GitHub setup, vault-grouped listings, and /settings; it refuses to guess when multiple keys are connected.
  • 1Password sync reuses saved Developer Environments, tap-protects SSH private keys, and skips Documents.
  • Button handling rejects bounce and cancelled requests; CTAPHID requests use separate channels and the authenticator locks when idle.
  • File replacement is atomic for configuration and cache files.

Written for commit c934fee. Summary will update on new commits.

View guided diff


CodeAnt-AI Description

Add nRF52840 support with passkeys, tap-based login, and commit signing

What Changed

  • Adds the Makerdiary nRF52840 dongle as the primary key board while keeping the ESP32-C6 supported. The nRF52840 accepts signed firmware updates.
  • Lets users manage passkeys with a separate FIDO PIN, enable tap-based sudo and lock-screen login, and sign Git commits with a tap.
  • Backups can include passkeys. Full restores require a five-second hold; users can also restore selected entries without replacing the rest of the key.
  • Groups synced entries by 1Password vault in the shell and brings key-management commands together under /settings.
  • A bouncing or cancelled button request no longer counts as approval, and the CLI asks users to choose when multiple keys are connected.

Impact

✅ Passkeys carried across compatible restores
✅ Tap-approved sudo and Git commits
✅ Fewer accidental button approvals

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

- New board folder firmware/boards/nrf52840-mdk-dongle: USB CDC, flash via
  the factory UF2 bootloader, image through llvm-objcopy; vkey check passed.
- Argon2 cost is the board's: Device::new takes KDF_COST (20 passes on the
  nRF52840, 48 on the C6-Zero); Cost::CURRENT is gone.
- INFO gains a chip line on the nRF52840 (build code, UICR APPROTECT, last
  reset cause); vkey info and vkey check print it. The chip is revision 2.
- CLI: refuse to guess between two plugged-in keys; refuse auth enable on a
  key without a chip key before writing anything; extract file::replace.
- Build: radio_base search for boards whose radio driver hides in the HAL;
  sign only Secure Boot boards; CI clippy over every board folder.
- Ignore RUSTSEC-2026-0344 (ruzstd via espflash, unreachable) with its exit
  condition in cli/.cargo/audit.toml.
- Docs: two boards in CLAUDE.md and the README, the nRF section of the threat
  model with the accepted revision-2 risk, docs2/roadmap.md.
- Re-sign the C6 images.
- Upstream Adafruit_nRF52_Bootloader 0.9.2 at a pinned commit, built with
  arm-gnu-toolchain 12.3.rel1 (sha256-checked) on Debian pinned by digest.
- Board definition read back from the factory 0.7.1 binary: 2886:F00F,
  UF2BOOT, the button on P0.18, the RGB LED.
- Patch 0001 drops CONFIG_GPIO_AS_PINRESET and CONFIG_NFCT_PINS_AS_GPIOS,
  so SystemInit writes no UICR.
- check.py fails the build on a UICR address in the code or a CF2 block
  that differs from the factory one. Two builds are byte-identical.
- tools/build-bootloader.sh builds by chip; nRF outputs stay in a temp dir
  until the fork is complete.
- Roadmap: phase 3 findings, constraints, milestones; check run 4 notes.
- 0002: no CDC, so no serial DFU; no BLE OTA, no SoftDevice update, no
  double reset, single-tap or skip magic. DFU mode only with the button at
  reset, no valid app, or the app's GPREGRET 0x57.
- 0003: the drive serves no flash; CURRENT.UF2 and its generator are gone.
- 0004: app blocks only to 0x1000..0x97000 (BOARD_APP_FLASH_END); a self-
  update stages at 0x8D000, below the vault.
- check.py also reads the disassembly for UICR immediates and movw/movt.
- Version 0.9.2-vkey.2 in INFO_UF2.TXT. Code 31312 -> 18096 B.
- Not installable yet: the self-update is unsigned and a partial UF2 still
  boots (milestone 3).
- src/signature.c: Monocypher's Ed25519 check with the message streamed
  from flash (slot header, then the image in 4 KiB pieces, watchdog fed).
- 0005: the app runs only if its signature verifies, checked once each boot
  with nothing written before the jump; DFU mode ends in a reset; a
  bootloader update is copied only if signed; the jump is always 0x1000,
  no SoftDevice is looked for; INFO_UF2.TXT names the key.
- test/run.sh, before every build: signature.c for Cortex-M4 under
  qemu-arm, 17 openssl-signed cases, streamed vs one-shot agreement.
- sign.sh + signed_image.py: sign, verify, wrap into a UF2; a test-key
  bootloader is signed only with ALLOW_TEST_KEY=1.
- The build needs bootloader/signing-key.pem or BOOTLOADER_TEST_KEY.
- memory.x ends the firmware at 0x96000 for the slot; image unchanged.
- roadmap: the key's second home is a .vkb that restores onto the C6;
  the key is an item on the nRF, backed up right after it is made.
- Version 0.9.2-vkey.3. Not run on a board yet (milestone 4).
- No board holds real secrets (2026-10-14); every feature is built for
  the nRF, with no fallback.
- The C6 takes check --wipe-everything for core changes once a .vkb
  holds vkey-signing, its Secure Boot key.
- roadmap: the migration shrinks to moving vkey-signing and taking C6
  flashing out of the CLI; the nRF bootloader key is an item on the nRF
  with a .vkb right after.
- 0006: tinyusb no longer asks a SoftDevice about the USB clock. It did
  whenever the app's word at 0x3004 held the SoftDevice magic, which
  would hang DFU mode. The only svc left is sd_mbr_command. 0.9.2-vkey.4.
- 0007 + VKEY_AS_APP in board.h: the same sources linked at 0x1000 to run
  under the factory bootloader: no COPY_BL, no settings write, a drive
  window of 0x40000..0x80000, label VKEYTEST. The real bootloader.bin is
  byte-identical with these hooks.
- test/app_test.c reports the bootloader's own signature checks with DWT
  cycles in INFO_UF2.TXT; test/board.py and board.sh make the UF2s and
  copy each only to the drive it is meant for.
- check.py --as-app: UICR search, no svc, ends below 0x40000. A movt of
  0x1000 whose low half is unknown now counts as a UICR address.
- On the board: the signed app accepted in 15.13 M cycles (~236 ms), an
  altered one refused; a signed self-update staged and accepted, an
  altered one refused. vkey restored from its signed UF2.
Milestone 3 of phase 6. fido-authenticator speaks CTAP2 over the
CTAPHID interface, Trussed runs inline in the main loop and keeps its
keys in a littlefs at 0xD8000..0xEC000. User presence is the vault's
tap; authenticatorReset takes the wipe's 5 s hold. U2F is not served.

Passkeys need more stack than the board had left beside Argon2's
static 128 KiB, so the core now takes that memory on the stack for one
derivation, and the nRF puts a 200 KiB stack at the bottom of RAM,
where an overflow resets instead of corrupting the statics. The C6's
image changed with the core; its stale signed copy leaves the
repository and vkey setup refuses a Secure Boot chip without one.

Accepted on the board: webauthn.io registration and login, fido2-token,
ctap2.py with signatures checked, check --wipe-everything with a
restore and a 1Password sync.
Milestone 4 of phase 6. vkey auth enable now makes a pam://<host>
credential on the key's FIDO2 side, tries it with an assertion, and
writes /etc/u2f_mappings and one pam_u2f line per PAM service. It
speaks CTAP2 itself: pamu2fcfg 1.1.0 demands an attestation
certificate this key does not have. The Ed25519 scheme sealed under
the chip key is gone (restorable from 4bd49f3); its category and
command numbers stay unused, and a restore leaves such an item out
and names it.

Two core fixes ride along. A request withdrawn while it waits for the
button ends refused and never counts as a press or a hold, which lets
a CTAPHID_CANCEL end a passkey's touch wait. A release now needs the
button up past the debounce, so a bounce mid-press is not two taps.

Accepted on the board: sudo and the lock screen by tap, the password
without the key, CANCEL answered in 0.2 s, check --wipe-everything
with a restore, a 1Password sync and selftest.
A backup now carries Trussed's flash as sealed 4 KiB sectors after the items,
under REGION_AAD plus the position. The core takes a fifth board trait,
hal::Attachment, and every backup session starts and ends in one place. The
board freezes FIDO during a session and restarts after a restore that changed a
sector. vkey restore --only adds named items from a .vkb beside the vault.

Accepted on the board: check, restore, sync and selftest; backup, passkey reset,
restore, then sudo and webauthn.io work again with nothing enrolled anew.
vkey-signing came onto the nRF from the C6's .vkb through restore --only, so
the C6's app and Secure Boot bootloader are signed without touching the C6. The
signed app returns to board.toml and vkey setup takes a Secure Boot chip again.
vkey commits enable makes an OpenSSH ecdsa-sk key on the nRF52840 and points
git's global signing at it; disable unsets commit.gpgsign and keeps the key. The
shell's /settings turns vkey auth and commit signing on and off, installing
libpam-u2f with apt when it is missing. Linux only; macOS and Windows are #18
and #19.
The vaults are a plain list of names and counts, led by 'all', every entry of
every vault. The commit signing key moves out of ~/.ssh, where GNOME Keyring's
agent takes it and then refuses to sign.
vkey passkey list|rm|pin speak CTAP2 credentialManagement and clientPin (PIN
protocol 1) over hidraw. The shell gets a passkey tab that asks the FIDO PIN once,
and /settings gets passkey-pin. The FIDO PIN stays apart from vkey's: the key
keeps it as a bare SHA-256. Also: github moves under the commits switch.
littlefs moves to 0xC8000..0xF4000 (44 blocks, up to the bootloader) and the
vault copies shrink to 24 sectors (0x97000..0xC8000). fido-authenticator refused
GitHub's passkey: 20 blocks left no room for its six-block margin. A restore now
leaves passkeys laid out for another count in the file instead of failing whole,
and vkey passkey list reports the room fido-authenticator estimates.
fido-authenticator 0.3 answers an empty enumerateRPs with a total of none rather
than NO_CREDENTIALS. A restore that carries passkeys says the key restarts only if
one changed, since the CLI cannot tell. The roadmap records milestone 6's run.
@codeant-ai

codeant-ai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

🤖 CodeAnt AI — Review Status

Status Commit Started (UTC) Finished (UTC)
✅ Incremental review completed e5e6f2a Oct 16, 2026 · 14:06 14:07
✅ Reviewed your PR 87c14a1 Oct 16, 2026 · 12:46 12:46
✅ Reviewed your PR 87c14a1 Oct 16, 2026 · 12:32 12:46

@codeant-ai

codeant-ai Bot commented Oct 16, 2026

Copy link
Copy Markdown

Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 34fd29e3-e374-4c64-8418-e683f2a4387d
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@codeant-ai codeant-ai Bot added the size:XXL This PR changes 1000+ lines, ignoring generated files label Oct 16, 2026

@cubic-dev-ai cubic-dev-ai 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.

16 issues found across 93 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="firmware/boards/nrf52840-mdk-dongle/bootloader/test/board.sh">

<violation number="1" location="firmware/boards/nrf52840-mdk-dongle/bootloader/test/board.sh:38">
P2: The factory INFO version line ends after `0.7.1` (CRLF), so this literal trailing-space requirement makes every app-family copy fail. Match a non-digit version boundary instead of requiring a space.</violation>
</file>

<file name="firmware/boards/nrf52840-mdk-dongle/bootloader/Dockerfile">

<violation number="1" location="firmware/boards/nrf52840-mdk-dongle/bootloader/Dockerfile:8">
P2: This pins an x86_64 toolchain without pinning the container platform, so the build fails on ARM64 Docker hosts when `arm-none-eabi-gcc` cannot execute. Pin the image to `linux/amd64` with emulation, or select a host-architecture-matched toolchain.</violation>
</file>

<file name="cli/src/prompt.rs">

<violation number="1" location="cli/src/prompt.rs:335">
P2: The switch row can exceed the terminal width because only the label is clipped; below 17 columns, the fixed prefix wraps and subsequent redraws clear too few rows. Clip the complete switch row to the available width and account for its physical rows when clearing.</violation>
</file>

<file name="cli/src/auth.rs">

<violation number="1" location="cli/src/auth.rs:152">
P1: `pam_u2f` cannot use this coordinate-only public key, so newly enrolled logins fall through to the password. Prefix the uncompressed point with `0x04` before encoding it.</violation>
</file>

<file name="firmware/boards/nrf52840-mdk-dongle/memory.x">

<violation number="1" location="firmware/boards/nrf52840-mdk-dongle/memory.x:32">
P2: A stack underflow overwrites the MBR-reserved forwarding data before reaching unmapped RAM, so the reset path can corrupt the MBR handoff state. Protect the reserved words with a guard before relying on this boundary to fault.</violation>
</file>

<file name="firmware/boards/nrf52840-mdk-dongle/bootloader/patches/0005-signed-images.patch">

<violation number="1" location="firmware/boards/nrf52840-mdk-dongle/bootloader/patches/0005-signed-images.patch:72">
P1: This accepts an app while leaving the writable flash after its declared length unauthenticated, so unsigned bytes in the app window can survive boot validation. Require a fixed-size signed app image or reject any non-erased tail before jumping.</violation>
</file>

<file name="cli/src/op.rs">

<violation number="1" location="cli/src/op.rs:260">
P2: The new branch marks an `SSHKEY` as secret, but `field_kind_of` still falls back to `String`, and `item_json` rewrites secret strings to `CONCEALED`; exporting therefore loses the SSH-specific private-key field type. Preserve `SSHKEY` as a distinct field kind and serialize it unchanged on export.</violation>

<violation number="2" location="cli/src/op.rs:999">
P2: Filtering documents here never cleans up entries owned by the retired `OP_DOC` bucket: `finish` now receives only `OP_ITEM` and `OP_ENV` phases, and pruning only visits each phase's bucket. Add an `OP_DOC` migration/cleanup before removing its phase, or previously imported documents remain on the device indefinitely.</violation>
</file>

<file name="cli/src/shell.rs">

<violation number="1" location="cli/src/shell.rs:1290">
P2: A vault literally named `all` opens the aggregate shelf instead, and `(no vault)` opens the unowned shelf, making either vault inaccessible. Keep the selector distinct from literal vault names or escape reserved names.</violation>

<violation number="2" location="cli/src/shell.rs:2468">
P2: The interactive shell discards the subprocess success flag, so failed auth, commit, and passkey-PIN operations are reported as successful. Check the `bool` returned by `outside` and return an error when it is false.</violation>
</file>

<file name="firmware/core/src/device.rs">

<violation number="1" location="firmware/core/src/device.rs:820">
P2: Backups of old vaults containing category-201 auth entries now fail instead of preserving or skipping those entries. Keep the legacy auth-key handling long enough to omit the retired entry, or migrate it explicitly before using the DEK-only path.</violation>

<violation number="2" location="firmware/core/src/device.rs:870">
P2: A backup with no emitted region blob does not replace or validate the destination attachment. Encode the source attachment count even when all sectors are blank, reject a different count, and clear all destination sectors for an empty source.</violation>
</file>

<file name="firmware/boards/nrf52840-mdk-dongle/bootloader/src/signature.c">

<violation number="1" location="firmware/boards/nrf52840-mdk-dongle/bootloader/src/signature.c:48">
P2: The verifier authenticates the image version but never enforces it, so previously signed vulnerable firmware and bootloaders can be replayed as downgrades. Compare the slot version against a protected current/minimum version before accepting app or bootloader updates.</violation>
</file>

<file name="tools/checks.sh">

<violation number="1" location="tools/checks.sh:182">
P2: The UF2 check verifies only the bootloader's informational fingerprint string, not the public key actually compiled into its verifier. Extract and compare the embedded key bytes, or validate the bootloader from the reproducible build, so a wrong-key bootloader cannot pass the hook.</violation>
</file>

<file name="cli/src/commits.rs">

<violation number="1" location="cli/src/commits.rs:78">
P2: `disable` removes `commit.gpgsign` unconditionally, so it can destroy a signing policy that vkey did not create. Only unset the setting when vkey owns the active configuration, or restore the previous value.</violation>
</file>

<file name="README.md">

<violation number="1" location="README.md:187">
P2: These credentials are non-resident, not absent from the key. Say they are excluded from the discoverable-passkey list so users do not misread reset or backup behavior.</violation>
</file>

Tip: Get a Slack DM when cubic finishes reviewing your PRs. Connect Slack

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread firmware/boards/nrf52840-mdk-dongle/bootloader/signing-key.sh Outdated
Comment thread firmware/boards/nrf52840-mdk-dongle/bootloader/sign-vaultkey.sh Outdated
Comment thread cli/src/auth.rs
Ok(format!(
"{user}:{},{},es256,+presence",
BASE64.encode(&credential.id),
BASE64.encode(&credential.public)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1: pam_u2f cannot use this coordinate-only public key, so newly enrolled logins fall through to the password. Prefix the uncompressed point with 0x04 before encoding it.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cli/src/auth.rs, line 152:

<comment>`pam_u2f` cannot use this coordinate-only public key, so newly enrolled logins fall through to the password. Prefix the uncompressed point with `0x04` before encoding it.</comment>

<file context>
@@ -180,34 +97,128 @@ pub fn enable(dev: &mut Device) -> Result<u8, Error> {
+    Ok(format!(
+        "{user}:{},{},es256,+presence",
+        BASE64.encode(&credential.id),
+        BASE64.encode(&credential.public)
+    ))
+}
</file context>

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Not changing this. pamu2fcfg writes an es256 key as the raw 64-byte x‖y (es256_pk_t, from fido_cred_pubkey_ptr), and pam_u2f reads it back with es256_pk_from_ptr, which takes that form; no 0x04 prefix is expected. A mapping written by this code was tried on the board: sudo with a tap succeeds.

Comment thread cli/src/auth.rs Outdated
Comment thread firmware/boards/nrf52840-mdk-dongle/bootloader/check.py Outdated
Comment thread docs/makerdiary.md Outdated
Comment thread README.md Outdated
Comment thread docs/threat-model.md Outdated
Comment thread CLAUDE.md
Comment thread CLAUDE.md Outdated
Comment thread cli/src/fido.rs
Comment thread cli/src/device.rs
Comment on lines +14 to +15
usb_vid = 0x1209
usb_pid = 0x0001

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: The pid.codes 1209:0001 test pair is shared, so another device using it may be auto-detected as this key and receive PIN or wipe commands.

Assessment: 🔴 Critical · 🔁 Occurrence: Rarely · 🏷️ Security

Use CodeAnt Skill Fix in Cursor Fix in VSCode Claude

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** firmware/boards/nrf52840-mdk-dongle/board.toml
**Line:** 14:15
**Comment:**
	*Security: The pid.codes `1209:0001` test pair is shared, so another device using it may be auto-detected as this key and receive PIN or wipe commands.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

The test pair is a deliberate choice for a personal device, and the CLI refuses to pick when two devices match. A USB ID is not authentication: a hostile device can present any pair, so a unique PID would not protect the PIN against one, and an unrelated gadget with this pair gets a framed request it cannot answer.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Customized review instruction saved!

Instruction:

Do not flag the shared pid.codes test VID/PID in this board configuration as a security issue: the CLI rejects ambiguous device matches, and USB IDs are not an authentication mechanism.

Applied to:

  • firmware/boards/nrf52840-mdk-dongle/board.toml

💡 To manage or update this instruction, visit: CodeAnt AI Settings

Comment thread cli/src/backup.rs
Comment thread cli/src/file.rs Outdated
Comment thread cli/src/op.rs
Comment thread cli/src/shell.rs
}
self.shelf = match vault.trim() {
"" => Shelf::Vaults,
ALL_VAULTS => Shelf::All,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: A vault named all is treated as the all-vaults sentinel, so selecting it opens every vault instead of listing that vault alone.

Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Logic error

Use CodeAnt Skill Fix in Cursor Fix in VSCode Claude

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** cli/src/shell.rs
**Line:** 1290:1290
**Comment:**
	*Logic Error: A vault named `all` is treated as the all-vaults sentinel, so selecting it opens every vault instead of listing that vault alone.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

True in principle: the row text doubles as the selector. It affects only grouping in the list, never what is stored or deleted, and no vault in use is named all. Left as is.

Comment thread cli/src/shell.rs
Comment thread tools/build-bootloader.sh
Comment on lines +42 to +44
for tool in docker openssl; do
command -v "$tool" >/dev/null || { echo "$tool is missing" >&2; exit 1; }
done

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: The ESP32-only path now aborts when host OpenSSL is missing, even though build_esp does not use it, so Docker-only bootloader builds regress.

Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Possible bug

Use CodeAnt Skill Fix in Cursor Fix in VSCode Claude

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** tools/build-bootloader.sh
**Line:** 42:44
**Comment:**
	*Possible Bug: The ESP32-only path now aborts when host OpenSSL is missing, even though `build_esp` does not use it, so Docker-only bootloader builds regress.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

The script builds the bootloaders of every board folder, and the nRF52840's build needs OpenSSL to check its key; asking for it up front fails before a long Docker build rather than in the middle of one. Every host this is built on has OpenSSL. Left as is.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Customized review instruction saved!

Instruction:

Keep the upfront OpenSSL availability check in the all-board bootloader build script so missing dependencies are caught before Docker builds begin.

Applied to:

  • tools/build-bootloader.sh

💡 To manage or update this instruction, visit: CodeAnt AI Settings

@vaulttec-dev

vaulttec-dev commented Oct 16, 2026 •

Copy link
Copy Markdown
Owner Author

Знахідки перевірок — як опрацьовано

  • «cargo-audit», «firmware image is reproducible and signed», «fmt, clippy, tests», «shellcheck, mypy», «actionlint, zizmor», «no forbidden marketing claim», «typos» — Node.js 20 deprecation of actions/checkout: not fixing in this PR, the pin is outside this change; it moves with the next Dependabot bump of the action.
  • «typos» exit code 2 — fixed in 6e4c343 (next push): _typos.toml now accepts COSE (RFC 9052), the two Adafruit bootloader identifiers, backticked commit hashes, and leaves a patch's context and removed lines (upstream text) unchecked.

Comment thread cli/src/fido.rs
Comment thread cli/src/fido.rs
Comment thread cli/src/fido.rs
Comment thread cli/src/fido.rs
Comment thread cli/src/fido.rs
Comment thread cli/src/fido.rs
Comment thread cli/src/fido.rs
Comment thread cli/src/fido.rs
Comment thread cli/src/fido.rs
Comment thread docs/makerdiary.md
Comment thread cli/src/auth.rs
Comment on lines +70 to +75
let existing = read_or_empty(Path::new(MAPPINGS))?;
file::replace(
Path::new(MAPPINGS),
with_mapping(&existing, &user, &mapping).as_bytes(),
0o644,
)?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: Concurrent enrolments can read the same old file, then overwrite each other; the last replacement drops the other user's mapping and breaks their key login.

Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Race condition

Use CodeAnt Skill Fix in Cursor Fix in VSCode Claude

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** cli/src/auth.rs
**Line:** 70:75
**Comment:**
	*Race Condition: Concurrent enrolments can read the same old file, then overwrite each other; the last replacement drops the other user's mapping and breaks their key login.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Not changing this: vkey auth enable runs under sudo, one user at a time on a personal machine, so two enrolments racing for /etc/u2f_mappings is not a case that occurs. The file itself is replaced whole, so a crash never leaves it half-written.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Customized review instruction saved!

Instruction:

Do not flag concurrent-enrolment lost-update risks in this auth flow; vkey auth enable runs under sudo with one user at a time on a personal machine.

Applied to:

  • cli/src/auth.rs

💡 To manage or update this instruction, visit: CodeAnt AI Settings

Comment thread cli/src/auth.rs
Comment thread cli/src/main.rs Outdated
@codeant-ai

codeant-ai Bot commented Oct 16, 2026

Copy link
Copy Markdown

CodeAnt Nitpicks

7 code suggestions

1. When a user already has a global SSH allowed-signers file, this replaces its path, so Git stops verifying signatures trusted by that file.

Logic error · cli/src/commits.rs:66


2. A relying party can store escape sequences in a passkey's user name; printing it raw lets vkey passkey manipulate terminal output or clipboard contents.

Security · cli/src/passkeys.rs:31


3. If the flashed board is absent while another key is the only match, find_port selects that key; its successful probe lets setup provision or wipe the wrong device.

Logic error · cli/src/setup.rs:205-209


4. A prior movw remains recorded after its register is overwritten, so a later movt can form a UICR address that this safety check misses.

Security · firmware/boards/nrf52840-mdk-dongle/bootloader/check.py:80-84


5. If gpg.format is changed away from ssh, this still reports commits enabled even though Git no longer signs with the configured security key.

Incorrect condition logic · cli/src/commits.rs:96-97


6. If available_ports() fails, unwrap_or_default() turns the failure into an empty list, so users see “no board found” instead of the enumeration error.

Logic error · cli/src/device.rs:281-282


7. Restoring passkeys reboots the FIDO side and invalidates its cached token, but this reconnect leaves self.passkeys intact; later deletion reuses the stale token and fails.

Stale reference · cli/src/shell.rs:2292

@vaulttec-dev

Copy link
Copy Markdown
Owner Author

CodeAnt Nitpicks (7)

  1. commits.rs:66 allowed-signers file — fixed: enable refuses a foreign gpg.ssh.allowedSignersFile.
  2. passkeys.rs:31 escape sequences — fixed: control characters in site and account names become ? where the CTAP answer is parsed.
  3. setup.rs:205-209 another key found alone after flashing — not changing in this PR: the board's port number may change after the reset, which is why the wait searches again; the flashed board is the one that just reset, and setup runs with the maintainer at the desk. Noted as a known limit.
  4. check.py:80-84 stale movw — fixed: any movt of 0x1000 now fails the check.
  5. commits.rs:96-97 gpg.format — fixed: enabled also requires gpg.format = ssh.
  6. device.rs:281-282 available_ports — fixed: an enumeration error is reported instead of "no board found".
  7. shell.rs:2292 stale passkey token after a restore — fixed: the token is dropped before the reconnect.

All fixes are in the next push.

@cubic-dev-ai cubic-dev-ai 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.

2 issues found across 27 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="docs/roadmap.md">

<violation number="1" location="docs/roadmap.md:45">
P2: This marks phase 5 skipped while the rule above still requires phases 3–5 before real secrets move to the nRF. Change the gate to phases 3–4 or explicitly say the skipped phase counts as complete.</violation>
</file>

<file name="cli/src/setup.rs">

<violation number="1" location="cli/src/setup.rs:209">
P1: `port_hint` is only a reusable device path, so this arm can select another key after the flashed key re-enumerates. `probe()` only checks the protocol, allowing a second key to pass before setup may wipe or reconfigure it. Match a stable device identity captured before flashing, or keep returning `ManyBoards` when the post-reset set is ambiguous.</violation>
</file>

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread cli/src/setup.rs
let port = match find_port() {
Ok(p) => p,
Err(Error::NoBoard) => port_hint.to_string(),
Err(Error::ManyBoards(ports)) if ports.iter().any(|p| p == port_hint) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1: port_hint is only a reusable device path, so this arm can select another key after the flashed key re-enumerates. probe() only checks the protocol, allowing a second key to pass before setup may wipe or reconfigure it. Match a stable device identity captured before flashing, or keep returning ManyBoards when the post-reset set is ambiguous.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cli/src/setup.rs, line 209:

<comment>`port_hint` is only a reusable device path, so this arm can select another key after the flashed key re-enumerates. `probe()` only checks the protocol, allowing a second key to pass before setup may wipe or reconfigure it. Match a stable device identity captured before flashing, or keep returning `ManyBoards` when the post-reset set is ambiguous.</comment>

<file context>
@@ -201,10 +201,14 @@ pub fn wait_for_firmware(port_hint: &str, timeout: Duration) -> Result<Device, E
         let port = match find_port() {
             Ok(p) => p,
             Err(Error::NoBoard) => port_hint.to_string(),
+            Err(Error::ManyBoards(ports)) if ports.iter().any(|p| p == port_hint) => {
+                port_hint.to_string()
+            }
</file context>

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Not changing this for now. The arm is taken only when the port named before flashing is back among the matches; for another key to sit there, it would have to drop off USB and re-enumerate into that name during the few seconds of the flash, while the flashed one came back elsewhere. Setup flashes only the C6 through espflash, and the other key in practice is the nRF, which never re-enumerates in that window. A check of the board's identity after the reset would close the gap fully; left open as a known limit.

Comment thread docs/roadmap.md Outdated
Comment thread cli/src/device.rs
Comment thread docs/roadmap.md

The order is fixed by what each step depends on. First the **bootloader** with no read-back
and signed images only: APPROTECT means nothing while UF2 hands out the flash. Then
**APPROTECT**. Then the **chip key**, skipped on this revision-2 chip (phase 5).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: This marks phase 5 skipped while the rule above still requires phases 3–5 before real secrets move to the nRF. Change the gate to phases 3–4 or explicitly say the skipped phase counts as complete.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/roadmap.md, line 45:

<comment>This marks phase 5 skipped while the rule above still requires phases 3–5 before real secrets move to the nRF. Change the gate to phases 3–4 or explicitly say the skipped phase counts as complete.</comment>

<file context>
@@ -33,14 +34,15 @@ notes, measurements and the factory state: `docs/makerdiary.md`.
 The order is fixed by what each step depends on. First the **bootloader** with no read-back
 and signed images only: APPROTECT means nothing while UF2 hands out the flash. Then
-**APPROTECT**. Then the **chip key**.
+**APPROTECT**. Then the **chip key**, skipped on this revision-2 chip (phase 5).
 
 ---
</file context>

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Not changing the gate: it is the maintainer's rule, and phase 5 being skipped on this revision-2 chip does not count as done. Real secrets wait for a chip key, which this board cannot have; buying a revision-3 board for that is tracked in issue #20.

@vaulttec-dev
vaulttec-dev merged commit c9a55ef into develop Oct 16, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL This PR changes 1000+ lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants