feat: thin agent CLI over BANALCore/BANALPublisher (#204) - #207
Merged
Merged
Conversation
banal vault | notes | show <id> | publish | doctor — read-mostly verification surface for agents, tests, and scripts. Same vault resolution (IntentVaultResolver), same NoteStore, same publish pipeline and statusCopy as the app; --vault DIR overrides, --json for machines, exit codes 0/1/2. Deliberately out of scope: editing, folder ops, watch, daemon. The editor stays the app; publish writes only the disposable stage and .publish trees. doctor reports the app's real environment: resolved vault + count, Boris/Oliver presence (absence is a warning, not a failure), and a contract row validating every published note's Boris entity id via the #202 boundary validator — the check CI structurally cannot run. New BANALCLI executable target ('banal'); 'banal-cli' keeps its documented launch-the-GUI behavior (#193). Tests cover parsing, every command's text+JSON shape, traversal guards, a stub-boris publish that behaves identically with or without boris installed, and doctor's healthy/degraded paths.
|
SummaryCoverage spans CLI vault inspection, note viewing, publishing, diagnostics, error handling, and separation of the desktop app from command-line use, including normal flows and adversarial path and filesystem edge cases. Overall health is weakened by security and data-integrity issues in the newly added inspection behavior. Not safe to merge yet — a high-severity path-containment flaw can expose files outside the selected vault, and a separate medium-severity issue allows read-only inspection to modify vault contents. Both are attributable to this PR and represent direct user-impacting behavior risks. Tests run by Ito
Tip Reply with @itoqa to send us feedback on this test run. |
![itoqa[bot]](https://avatars.githubusercontent.com/in/2622614?s=60&v=4){kind=small}
| return store | ||
| } | ||
|
|
||
| private static func noteURL(for id: String, in configuration: VaultConfiguration) throws -> URL { |
There was a problem hiding this comment.
Symlinked notes can expose outside files
What failed: The command should reject a note path that resolves outside the selected vault. Its path check accepts a link whose visible path is inside the vault, so show can return the contents of an outside file.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: High
- Impact: A user can open a note link inside the vault and see the contents of a file outside it. This can expose private files whenever such a link is present.
- Steps to Reproduce:
- Create a temporary vault directory and a separate readable Markdown file outside that directory.
- Create a symlink named linked.md inside the vault that points to the outside Markdown file.
- Run banal show linked.md --vault .
- Observe that the command accepts the path and returns the outside file contents instead of rejecting the link.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: BanalCLI.noteURL(for:in:) checks the extension and rejects lexical absolute paths at Sources/BANALCLI/BanalCLI.swift:302-308. It then builds a candidate with appendingPathComponent(id).standardizedFileURL and compares candidate.path with configuration.rootURL.standardizedFileURL.path using a string prefix at lines 309-312. standardization removes components such as '..' but does not resolve filesystem symlinks. The later fileExists check and returned URL at lines 314-317 still refer to the vault-local symlink path, and Foundation follows that symlink when show reads the URL. The PR diff adds the entire BanalCLI implementation, including these lines, so the missing filesystem-resolution check is introduced by this PR. A targeted fix is to use resolvingSymlinksInPath on the root and candidate, reject a resolved candidate outside the resolved root, and read the validated resolved URL.
- Why this is likely a bug: The command is documented as a read-only view of a selected vault, so a note identifier that points outside that vault should not disclose another file. The code checks only the spelling of the path, while normal file reads follow symlinks; an attacker or accidental link can therefore bypass the intended boundary. Resolving both paths before containment checking is a small, local correction.
Relevant code
Sources/BANALCLI/BanalCLI.swift:302-317
private static func noteURL(for id: String, in configuration: VaultConfiguration) throws -> URL {
guard NoteLanguage(pathExtension: (id as NSString).pathExtension) != nil else {
throw CLIFailure("\"\(id)\" has no .md/.textile/.cook extension")
}
guard !id.hasPrefix("/") else {
throw CLIFailure("note id must stay inside the vault")
}
let root = configuration.rootURL.standardizedFileURL.path
let candidate = configuration.rootURL.appendingPathComponent(id).standardizedFileURL
guard candidate.path.hasPrefix(root + "/") else {
throw CLIFailure("note id must stay inside the vault")
}
guard FileManager.default.fileExists(atPath: candidate.path) else {
throw CLIFailure("no note \"\(id)\" in \(root)")
}
return candidate
}Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**High severity — Symlinked notes can expose outside files**
**What failed:** The command should reject a note path that resolves outside the selected vault. Its path check accepts a link whose visible path is inside the vault, so show can return the contents of an outside file.
- **Impact:** A user can open a note link inside the vault and see the contents of a file outside it. This can expose private files whenever such a link is present.
- **Steps to reproduce:**
1. Create a temporary vault directory and a separate readable Markdown file outside that directory.
2. Create a symlink named linked.md inside the vault that points to the outside Markdown file.
3. Run banal show linked.md --vault <vault-directory>.
4. Observe that the command accepts the path and returns the outside file contents instead of rejecting the link.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** BanalCLI.noteURL(for:in:) checks the extension and rejects lexical absolute paths at Sources/BANALCLI/BanalCLI.swift:302-308. It then builds a candidate with appendingPathComponent(id).standardizedFileURL and compares candidate.path with configuration.rootURL.standardizedFileURL.path using a string prefix at lines 309-312. standardization removes components such as '..' but does not resolve filesystem symlinks. The later fileExists check and returned URL at lines 314-317 still refer to the vault-local symlink path, and Foundation follows that symlink when show reads the URL. The PR diff adds the entire BanalCLI implementation, including these lines, so the missing filesystem-resolution check is introduced by this PR. A targeted fix is to use resolvingSymlinksInPath on the root and candidate, reject a resolved candidate outside the resolved root, and read the validated resolved URL.
- **Why this is likely a bug:** The command is documented as a read-only view of a selected vault, so a note identifier that points outside that vault should not disclose another file. The code checks only the spelling of the path, while normal file reads follow symlinks; an attacker or accidental link can therefore bypass the intended boundary. Resolving both paths before containment checking is a small, local correction.
**Relevant code:**
`Sources/BANALCLI/BanalCLI.swift:302-317`
~~~swift
private static func noteURL(for id: String, in configuration: VaultConfiguration) throws -> URL {
guard NoteLanguage(pathExtension: (id as NSString).pathExtension) != nil else {
throw CLIFailure("\"\(id)\" has no .md/.textile/.cook extension")
}
guard !id.hasPrefix("/") else {
throw CLIFailure("note id must stay inside the vault")
}
let root = configuration.rootURL.standardizedFileURL.path
let candidate = configuration.rootURL.appendingPathComponent(id).standardizedFileURL
guard candidate.path.hasPrefix(root + "/") else {
throw CLIFailure("note id must stay inside the vault")
}
guard FileManager.default.fileExists(atPath: candidate.path) else {
throw CLIFailure("no note \"\(id)\" in \(root)")
}
return candidate
}
~~~
| } | ||
| } | ||
|
|
||
| private static func vault(_ invocation: Invocation, out: (String) -> Void) throws -> Int32 { |
There was a problem hiding this comment.
Read-only commands change vault files
What failed: The inspection commands are expected to leave every vault file unchanged, but opening the vault can create directories, write configuration, and seed Welcome.md.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: A user checking an existing vault can find new folders or files, including a configuration file or Welcome note, even though the command is read-only. This can undermine trust in the vault and may require cleanup, but no data loss is shown.
- Steps to Reproduce:
- Prepare an existing vault with nested notes and custom configuration, and record every file's path, size, modification time, and content hash.
- Run
banal vault,banal notes,banal show <id>, andbanal doctoragainst that vault, including their JSON forms. - Scan the vault again and compare the file list and file contents with the original snapshot.
- Observe that the inspection path can add
.banal/config.json,assets,.banal, orWelcome.mdwhen those items are missing.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: The defect is established by the production source even though the native runtime could not be built in the Linux test container because the Apple-only
UniformTypeIdentifiersmodule was unavailable. In the PR-addedSources/BANALCLI/BanalCLI.swift,vault()callsopenStore(configuration)at lines 151-155,notes()callsopenStore(resolveVault(...))at lines 164-167,publish()calls it at lines 201-207, anddoctor()calls it at lines 233-241.show()resolves the same vault at lines 182-191 and uses the shared note path.openStore()at lines 295-300 constructsNoteStoreand immediately callsstore.open().Sources/BANALCore/NoteStore.swift:156-165checks that the root exists, then unconditionally callsVaultBootstrap.prepare, loads configuration, reloads notes, and starts monitoring.Sources/BANALCore/VaultConfiguration.swift:149-176makes that preparation mutating: it creates the assets and metadata directories, writes.banal/config.jsonwhen absent at lines 154-158, and writesWelcome.mdwhen no top-level note exists at lines 160-175. The CLI's new inspection commands therefore violate the PR-added read-mostly contract for valid existing vaults. The smallest practical fix is to give read-only CLI operations a non-bootstrapping open path that validates and loads existing metadata without callingVaultBootstrap.prepare; keep bootstrap behavior for app initialization and explicitly mutating setup flows. - Why this is likely a bug: This is a real application defect rather than a setup failure: the source unconditionally invokes a function whose documented behavior is to create directories and files. The PR explicitly describes
banalas read-mostly and says it never creates or changes notes, while its newly added command handlers all use the mutating initialization path. The Linux build limitation prevented observing the before-and-after snapshot, but it does not change the reachable production control flow. A targeted non-mutating read path for the inspection commands fixes the contract without changing the app's normal bootstrap behavior.
Relevant code
Sources/BANALCLI/BanalCLI.swift:151-167
private static func vault(_ invocation: Invocation, out: (String) -> Void) throws -> Int32 {
let configuration = try resolveVault(invocation.vaultPath)
let count = try MainActor.assumeIsolated {
try openStore(configuration).notes.count
}
...
}
private static func notes(_ invocation: Invocation, out: (String) -> Void) throws -> Int32 {
let storeNotes = try MainActor.assumeIsolated { () -> [Note] in
try openStore(resolveVault(invocation.vaultPath)).notes
Sources/BANALCLI/BanalCLI.swift:295-300
@MainActor
private static func openStore(_ configuration: VaultConfiguration) throws -> NoteStore {
let store = NoteStore(configuration: configuration, monitor: nil)
try store.open()
return store
}Sources/BANALCore/NoteStore.swift:156-165
public func open() throws {
var isDirectory: ObjCBool = false
if !fileManager.fileExists(atPath: configuration.rootURL.path, isDirectory: &isDirectory) || !isDirectory.boolValue {
throw NoteStoreError.vaultNotDirectory(configuration.rootURL)
}
try VaultBootstrap.prepare(configuration, fileManager: fileManager)
configuration = VaultBootstrap.load(from: configuration.rootURL, fileManager: fileManager)
rootMissing = false
try reloadAll()
startMonitor()
}Sources/BANALCore/VaultConfiguration.swift:149-176
public static func prepare(_ configuration: VaultConfiguration, fileManager: FileManager = .default) throws -> URL {
try fileManager.createDirectory(at: configuration.rootURL, withIntermediateDirectories: true)
try fileManager.createDirectory(at: configuration.assetsURL, withIntermediateDirectories: true)
try fileManager.createDirectory(at: configuration.metadataURL, withIntermediateDirectories: true)
if !fileManager.fileExists(atPath: configuration.configURL.path) {
...
try data.write(to: configuration.configURL, options: .atomic)
}
...
if !hasNote {
...
try Data(document.utf8).write(to: welcomeURL, options: .atomic)
}Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Read-only commands change vault files**
**What failed:** The inspection commands are expected to leave every vault file unchanged, but opening the vault can create directories, write configuration, and seed `Welcome.md`.
- **Impact:** A user checking an existing vault can find new folders or files, including a configuration file or Welcome note, even though the command is read-only. This can undermine trust in the vault and may require cleanup, but no data loss is shown.
- **Steps to reproduce:**
1. Prepare an existing vault with nested notes and custom configuration, and record every file's path, size, modification time, and content hash.
2. Run `banal vault`, `banal notes`, `banal show <id>`, and `banal doctor` against that vault, including their JSON forms.
3. Scan the vault again and compare the file list and file contents with the original snapshot.
4. Observe that the inspection path can add `.banal/config.json`, `assets`, `.banal`, or `Welcome.md` when those items are missing.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** The defect is established by the production source even though the native runtime could not be built in the Linux test container because the Apple-only `UniformTypeIdentifiers` module was unavailable. In the PR-added `Sources/BANALCLI/BanalCLI.swift`, `vault()` calls `openStore(configuration)` at lines 151-155, `notes()` calls `openStore(resolveVault(...))` at lines 164-167, `publish()` calls it at lines 201-207, and `doctor()` calls it at lines 233-241. `show()` resolves the same vault at lines 182-191 and uses the shared note path. `openStore()` at lines 295-300 constructs `NoteStore` and immediately calls `store.open()`. `Sources/BANALCore/NoteStore.swift:156-165` checks that the root exists, then unconditionally calls `VaultBootstrap.prepare`, loads configuration, reloads notes, and starts monitoring. `Sources/BANALCore/VaultConfiguration.swift:149-176` makes that preparation mutating: it creates the assets and metadata directories, writes `.banal/config.json` when absent at lines 154-158, and writes `Welcome.md` when no top-level note exists at lines 160-175. The CLI's new inspection commands therefore violate the PR-added read-mostly contract for valid existing vaults. The smallest practical fix is to give read-only CLI operations a non-bootstrapping open path that validates and loads existing metadata without calling `VaultBootstrap.prepare`; keep bootstrap behavior for app initialization and explicitly mutating setup flows.
- **Why this is likely a bug:** This is a real application defect rather than a setup failure: the source unconditionally invokes a function whose documented behavior is to create directories and files. The PR explicitly describes `banal` as read-mostly and says it never creates or changes notes, while its newly added command handlers all use the mutating initialization path. The Linux build limitation prevented observing the before-and-after snapshot, but it does not change the reachable production control flow. A targeted non-mutating read path for the inspection commands fixes the contract without changing the app's normal bootstrap behavior.
**Relevant code:**
`Sources/BANALCLI/BanalCLI.swift:151-167`
~~~swift
private static func vault(_ invocation: Invocation, out: (String) -> Void) throws -> Int32 {
let configuration = try resolveVault(invocation.vaultPath)
let count = try MainActor.assumeIsolated {
try openStore(configuration).notes.count
}
...
}
private static func notes(_ invocation: Invocation, out: (String) -> Void) throws -> Int32 {
let storeNotes = try MainActor.assumeIsolated { () -> [Note] in
try openStore(resolveVault(invocation.vaultPath)).notes
~~~
`Sources/BANALCLI/BanalCLI.swift:295-300`
~~~swift
@MainActor
private static func openStore(_ configuration: VaultConfiguration) throws -> NoteStore {
let store = NoteStore(configuration: configuration, monitor: nil)
try store.open()
return store
}
~~~
`Sources/BANALCore/NoteStore.swift:156-165`
~~~swift
public func open() throws {
var isDirectory: ObjCBool = false
if !fileManager.fileExists(atPath: configuration.rootURL.path, isDirectory: &isDirectory) || !isDirectory.boolValue {
throw NoteStoreError.vaultNotDirectory(configuration.rootURL)
}
try VaultBootstrap.prepare(configuration, fileManager: fileManager)
configuration = VaultBootstrap.load(from: configuration.rootURL, fileManager: fileManager)
rootMissing = false
try reloadAll()
startMonitor()
}
~~~
`Sources/BANALCore/VaultConfiguration.swift:149-176`
~~~swift
public static func prepare(_ configuration: VaultConfiguration, fileManager: FileManager = .default) throws -> URL {
try fileManager.createDirectory(at: configuration.rootURL, withIntermediateDirectories: true)
try fileManager.createDirectory(at: configuration.assetsURL, withIntermediateDirectories: true)
try fileManager.createDirectory(at: configuration.metadataURL, withIntermediateDirectories: true)
if !fileManager.fileExists(atPath: configuration.configURL.path) {
...
try data.write(to: configuration.configURL, options: .atomic)
}
...
if !hasNote {
...
try Data(document.utf8).write(to: welcomeURL, options: .atomic)
}
~~~… from #205 Beau's parallel CLI implementation (#205) had four ideas worth more than ours: - doctor now distinguishes a configured-but-not-executable engine (fail: a config error) from an engine that was never configured (warn: the builtin path is a healthy choice). - Exit code 64 when doctor finished with warnings — agents can tell degraded-but-working from broken without parsing output. - notes gains --published; note JSON gains bytes (file size). - publish refreshes security-scoped compiler bookmarks before resolving engines, matching the app's own courtesy.
drawmeanelephant
added a commit
that referenced
this pull request
Aug 25, 2026
main picked up the agent CLI (#207) and the identity-contract fix (#206) while this branch carried the AppleScript dictionary. Both sides appended a section after Publish — Command line and Scripting now coexist — and the STATUS verification-tooling exception appears once, with both surfaces listed.
|
Diff SummaryCoverage spans identity validation and boundary cases, note selection and ordering, publishing outputs and source preservation, product separation, and clear error handling. Adversarial coverage also examined malformed or oversized names and overlapping publishing, with overall behavior broadly healthy aside from a publishing edge case involving name collisions near the length limit. Merge with caution — a medium-severity failure attributable to this PR remains in the publishing path, where collision suffixes can produce invalid names and unusable output. Separate high-severity concurrency findings were identified but are not attributable to this PR and are caveats for later. Tests run by Ito
Findings dismissed by reviewerBelow are prior failures a reviewer explicitly dismissed. They were not retested and are not counted as outstanding failures:
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟠 Concurrent publishing can overwrite results
Evidence Package🟠 Overlapping publishes can overwrite each other
Evidence Package🟡 Long note names create invalid published IDs
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #204. Stacked on #206 (the #202 identity fix —
doctor's contract row reuses its validator). Retarget tomainonce #206 merges.Scope
Exactly the v1 surface proposed in the issue, plus the
doctorcommand from the follow-up comment:BANALCLI→ productbanal.banal-clikeeps its documented launches-the-GUI behavior, so README: CLT-only build fails without warning; banal-cli launches the GUI #193's doc stays true.--vault DIRis the agent-reliable path.error 0wrap from the issue.IntentVaultResolver,NoteStore,VaultBootstrap.load,PublishConfiguration.default,BANALPublisher.make.Deliberately NOT here
No editing, no folder ops, no watch, no daemon. Publish writes only
.banal/stageand.publish. The editor is the app.doctor
The row the follow-up comment asked for:
Missing engines are warnings (builtin still publishes); only vault/contract failures exit non-zero.
Tests + docs
15 CLI tests: parsing/usage errors, every command text+JSON shape, path-traversal guards on
show, a contract-checking stub-boris publish that behaves identically with or without boris installed (CI has none; agents might), doctor healthy/degraded paths.swift test: full suite green (306 core + publisher + 15 CLI).README gains a Command line section; docs/STATUS.md records the written exception for verification tooling per AGENTS.md; CHANGELOG updated.