fix: an interactive run stops opening prompts nobody can answer, and stops reporting side effects it never had - #1632
Open
chhoumann wants to merge 8 commits into
Open
fix: an interactive run stops opening prompts nobody can answer, and stops reporting side effects it never had#1632chhoumann wants to merge 8 commits into
chhoumann wants to merge 8 commits into
Conversation
…r had A choice run answered "success" to the question "did you finish?", which is not the question an automation asks - it asks "did anything land?". Those had come apart in two shipped configurations that need no interactivity at all: - a Capture whose formatted payload is empty deliberately leaves the file untouched (inserting would add a blank line or, on `currentLine`, DELETE the selection) and says so in a notice, but still recorded a plain success; - a Template set to "Do nothing" when the file already exists is, by name, a no-op, and recorded the same. Both answered `verified:true` with a file path over a byte-identical vault, so a caller counting captures or writing an idempotency marker recorded work that never happened. `ChoiceOutcome`'s success variant now carries a `ChoiceEffect` - `created` / `changed` / `unchanged` - and the recorder takes it as a REQUIRED argument so every call site has to state its claim rather than inherit a positive one by omission. That is what surfaced the two Template sites: of the four success sites in the tree, the two nobody had looked at were the two that commit nothing. The claim is about persisted bytes, never about the payload. An empty payload can still legitimately create a note (create-if-not-found, possibly with a rendered template body), and a non-empty one can still write what was already there - so Capture compares the file's prior content, plus any front-matter post-processing, instead of asking whether the payload was blank. Two consequences worth naming: - The canvas path now checks for the no-op BEFORE it writes. It re-serialises the whole `.canvas` JSON, so writing identical card text still rewrote the file - which would have made the `unchanged` it was about to report false. - An `unchanged` run no longer closes the outcome. The close-guard exists to stop an automation retrying and DUPLICATING a side effect; a run that left none has nothing to protect, and closing there would only hide a real post-commit failure behind a benign "nothing to capture". Refs #1615
…`verified` The `effect` a run had is now on every terminal frame the CLI, the interactive bridge and the x-callback emit, so a caller can finally ask "did anything land?" instead of inferring it from "did the run finish?". `verified` is deliberately left exactly as it was. It already means "QuickAdd could not confirm this - go look", and the handler's own comment invites retry logic on `verified:false`. Folding "we confirmed, and nothing happened" into that same value would make a correct, permanently-unchanging run look retryable forever - the one change here that would have broken a shipped contract silently. So the new fact gets a new key rather than a new meaning for an old one. `unknown` is stated rather than omitted, on both legacy tails. A missing key reads as `false` to `jq '.effect'` and to `!res.effect` alike, which is exactly the wrong direction: it would turn "QuickAdd did not look" into "nothing happened". The interactive legacy tail emitted neither flag before, so a client could not tell it apart from the verified frame at all; it now says both. `capabilities` gains `outcome-effect` so a bridge client can feature-detect this instead of probing for the key, and the x-callback success params carry `effect` too. That last one is not a leak: it is a three-token enum, and the same callback already sends the note's path and the vault name - withholding it would leave the Shortcut that writes an idempotency marker on `status=success`, which is the automation this issue is about, still marking captures that never happened. Refs #1615
…er on the desktop
`quickadd:interactive` promises to forward a run's prompts to the connected
client. It kept that promise for every prompt a script or the formatter opens,
because each of those hand-wrote an `if (promptProvider)` branch at its own call
site. The engines never got one, so a Template whose target note already existed
opened its chooser in Obsidian while the client's `/poll` returned nothing - and
the run sat there until someone walked past the machine, on exactly the headless
setups this seam exists for.
`routePrompt` is a seam rather than a seventh hand-written branch, because
"remember to add the branch" had already been missed six times. It deliberately
owns only what is uniform: the ORDER of the three destinations and the
`isCancellationError` -> UserCancelError mapping every site was repeating. Each
site still passes three closures and keeps its own modal call verbatim, because
the variance here is entirely in modal construction - and because the headless
branch is not uniform either. Most sites abort; MacroChoiceEngine legitimately
runs a sole exported member without asking, and a fixed ladder would have
regressed that.
Engine replies are also validated, which the in-app modal gets for free:
GenericSuggester can only ever resolve an element of its own list, and routing
the prompt must not widen a closed list into free text just because the answer
now arrives over HTTP. `RemotePromptProvider.suggester` returns any string
verbatim and maps a missing value to `""`; both are correct for the script
callers it was written for and dangerous here - `""` reaches
`getFileExistsMode("")`, whose internal `Unknown file exists mode: ` is what a
client would have seen. An off-list reply now ends the run naming the fix, and an
empty one is treated as the dismissal it is. The lenient path is untouched for
the script and formatter callers that depend on it.
Session teardown now rejects pending prompts with ChoiceAbortError instead of a
bare Error. That only became load-bearing here: `isCancellationError` is false
for a plain Error, so once engine prompts travel this bridge, a client that
simply stopped polling would have been reported as a run FAILURE - a red notice
on a desktop nobody is sitting at.
Verified live in an isolated vault: the chooser now arrives over `/poll` with an
empty `.modal-container` list in Obsidian, an off-list reply is refused, a
dismissal ends the run like Escape does, and `POST /abort` reports
`interrupted:1` where #1614 documented `0`.
Refs #1614
…lient
Four more prompts an engine opens now go to the connected client instead of the
desktop. Each was confirmed to strand a real run first, by starting
`quickadd:interactive` in an isolated vault and checking both sides: the client's
`/poll` returned nothing while `document.querySelectorAll(".modal-container")`
showed the modal sitting in Obsidian.
- the folder chooser (TemplateEngine)
- the note-discovery picker (templateNoteDiscovery) - the highest-value one, since
the one-page preflight deliberately filters its requirement out, so an
interactive run collected NOTHING and walked into a desktop list of every note
- the heading picker (CaptureChoiceEngine)
- the user-script member picker (MacroChoiceEngine)
The capture-target pickers are deliberately NOT converted. A remote run forces the
one-page preflight, which already collects the target as a first-class
`__qa.captureTargetFilePath` requirement - verified live: the client receives a
`form` prompt and no modal opens. Routing them would have meant bypassing
`confinePreselectedToScope`, the confinement this codebase already requires for a
capture target that arrives from outside the picker, to fix a symptom that does
not occur.
Three details the conversion forced:
- The discovery picker's `display` is a fuzzy-SEARCH blob (basename + path +
aliases) and the visible row comes from `renderItem`, which does not travel. It
now carries an explicit `title`, so a client shows "Areas/Work/Tom.md" rather
than "Tom Areas/Work/Tom.md Thomas".
- The folder chooser's re-prompt loop is unbounded because a Notice tells the user
why their pick was refused. A client gets no Notice, so an identical re-prompt is
indistinguishable from a hang; a remote run is bounded at three attempts and then
ends with the text the Notice carries, via one shared message.
- The member picker validates with `Object.hasOwn` before indexing the script
module, since a reply that used to come from a fixed key list now arrives over
the wire.
Also folds in a one-line guard for the AI tool-confirmation modal, which is worse
than #1614 rather than an instance of it: it has neither a provider route nor a
non-interactive guard, so a plain headless `quickadd:run` of a Macro whose script
calls `quickAddApi.ai.agent` with a non-readOnly tool never returned at all.
Refs #1614
…ort now reaches Refs #1614
…eview found false
An adversarial pass took apart the previous commit's reasoning, and it was right
about the biggest piece: the capture-target pickers DO strand a remote run. The
justification for leaving them - "the forced one-page preflight already collects
the target" - holds only while the preflight actually runs and can classify the
target. Two shipped configurations break it, both reproduced live:
- "Capture to" carrying format syntax (`Journal/{{DATE:YYYY-MM}}/`):
`classifyCaptureTargetScope` returns null for any target containing `{{...}}`,
so no requirement is collected and the picker opens at run time.
- a choice whose one-page input is set to "never": `shouldUseOnePager` is false
even for a remote run, so there is no preflight at all.
Both now route, and the confinement the earlier design review warned about is
preserved rather than bypassed. The folder-scoped picker needed nothing extra -
every reply is re-prefixed into the folder afterwards, exactly what
`confinePreselectedToScope` does. The tag/property picker did: nothing downstream
re-confines it, and in Obsidian the rule is enforced structurally by `valueExists`
suppressing the "Create new note" row for a name that already exists, so a typed
value can only ever create a NEW note. A routed reply naming an existing note the
scope does not match is now refused with that same rule.
Three other corrections:
- **`unchanged` closes the outcome again.** The previous commit exempted it on the
grounds that a no-op "left nothing a retry could duplicate". False: the run
continues past the commit point into the append-link step, which writes to a
DIFFERENT note. Letting a later failure overwrite the recorded success put the
caller straight back to retrying a run that already had side effects.
- **`effect` is about the choice's TARGET, not the whole vault**, and now says so.
Append-link and copy-link are real writes this flag does not describe; claiming
the vault was unchanged while a link was appended elsewhere was the same kind of
overclaim the flag exists to remove.
- **A post-commit rewrite counts as a change.** Whole-file Templater
(`afterCapture: "wholeFile"`) rewrites the note AFTER the bytes were compared, so
a note containing `<% %>` really did change even when the payload was a no-op.
Plus: an empty reply is checked BEFORE list membership, because a folder list can
legitimately contain "" and that is the one reply whose two readings differ most
("the client sent nothing" vs "create at the vault root"); the folder chooser's
remote abort carries the reason that actually fired instead of always blaming the
allowed roots; and two comments and the CLI docs that the previous commit left
describing behaviour the code no longer has.
Refs #1614
Refs #1615
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughQuickAdd now reports whether successful Template and Capture runs created, changed, or left files unchanged. Engine prompts route to remote clients, headless handlers, or in-app suggesters with stricter reply validation and typed abort handling. ChangesOutcome effects and prompt routing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant registerQuickAddCliHandlers
participant routePrompt
participant TemplateChoiceEngine
participant ChoiceOutcomeRecorder
Client->>registerQuickAddCliHandlers: start interactive run
registerQuickAddCliHandlers->>TemplateChoiceEngine: execute choice
TemplateChoiceEngine->>routePrompt: request engine prompt
routePrompt-->>Client: forward prompt
Client-->>routePrompt: return validated choice
TemplateChoiceEngine->>ChoiceOutcomeRecorder: record effect
ChoiceOutcomeRecorder-->>registerQuickAddCliHandlers: return verified outcome
registerQuickAddCliHandlers-->>Client: return effect
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Deploying quickadd with
|
| Latest commit: |
a3004c5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://47d980f8.quickadd.pages.dev |
| Branch Preview URL: | https://chhoumann-1607-interactive-p.quickadd.pages.dev |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0254d02420
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…k the docs unreleased
Review caught a real hole in the previous commit's reasoning. Its comment claimed
"a client that really means the root row replies with its index token, which is
unambiguous" - but the provider decodes that token back to the raw value BEFORE
this module sees it. So picking the vault-root row and answering nothing at all
both arrived here as `""`, and the empty-is-a-cancel rule made a legitimate
selection abort the run.
The channel now hands the provider its own opaque row handles instead of the real
values, and maps a handle back afterwards. That makes "the client picked row N"
structurally distinct from "the client said nothing", whatever row N's value
happens to be - which is what the folder chooser needs, since "" really is one of
its rows when a `{{VALUE:sub}}` entry resolves to nothing.
Also adds the `:::note[Available in the next release]` callouts AGENTS.md requires
for documented-but-unshipped behaviour: docs deploy from master ahead of releases,
so `effect` and the forwarded engine prompts both need one.
Refs #1614
Refs #1615
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/engine/templateNoteDiscovery.test.ts (1)
25-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
IN_APP_RUNtest sentinel across two files. Both test files define the identicalconst IN_APP_RUN = {} as never;(with the same comment) to simulate an ordinary in-appPromptRoutingContext. Sharing one fixture avoids the two copies drifting if the sentinel's type or rationale ever needs to change.
src/engine/templateNoteDiscovery.test.ts#L25-L27: importIN_APP_RUNfrom a shared test-fixture module instead of declaring it locally.src/engine/templateNoteDiscovery.audit-template.test.ts#L21-L24: same — import from the shared fixture instead of redeclaring.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/engine/templateNoteDiscovery.test.ts` around lines 25 - 27, Replace the local IN_APP_RUN declaration in src/engine/templateNoteDiscovery.test.ts lines 25-27 with an import from a shared test-fixture module. Apply the same change in src/engine/templateNoteDiscovery.audit-template.test.ts lines 21-24, removing each duplicated comment and sentinel while preserving the existing test usage.src/engine/TemplateEngine.ts (1)
288-309: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAttempt-counting condition is correct but hard to read at a glance.
executor.promptProvider && remoteAttempts-- <= 0relies on post-decrement + short-circuit ordering to allow exactlyMAX_REMOTE_FOLDER_ATTEMPTSprompts before aborting. It checks out (verified by hand-tracing all iterations), but a maintainer skimming this later could easily misread it as an off-by-one.♻️ Optional clarity refactor
- let remoteAttempts = executor.promptProvider ? MAX_REMOTE_FOLDER_ATTEMPTS : 0; + let remoteAttemptsLeft = executor.promptProvider ? MAX_REMOTE_FOLDER_ATTEMPTS : Infinity; let lastRejection: string | null = null; for (;;) { - if (executor.promptProvider && remoteAttempts-- <= 0) { + if (remoteAttemptsLeft <= 0) { throw new ChoiceAbortError( lastRejection ?? folderNotAllowedMessage(context.allowedRoots), ); } + remoteAttemptsLeft--; const raw = await this.promptForFolder(context, executor);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/engine/TemplateEngine.ts` around lines 288 - 309, Clarify the attempt tracking in promptUntilAllowed by replacing the post-decrement condition on remoteAttempts with an explicit, readable check and decrement that preserves exactly MAX_REMOTE_FOLDER_ATTEMPTS remote prompts before throwing ChoiceAbortError. Keep the existing local-provider behavior and lastRejection fallback unchanged.src/engine/CaptureChoiceEngine.effect.test.ts (1)
80-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMock path style differs from the neighbouring mocks.
Every other
vi.mockhere uses a relative specifier (../main,../utilityObsidian); this one usessrc/gui/InputSuggester/inputSuggester. It only mocks the right module if thesrc/*alias resolves to the same id the engine imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/engine/CaptureChoiceEngine.effect.test.ts` around lines 80 - 82, Update the vi.mock declaration for InputSuggesterMock to use the same relative module specifier style as the neighboring mocks, matching the import path used by the engine so the intended module is reliably mocked.src/engine/CaptureChoiceEngine.ts (2)
596-603: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFold the duplicated
frontmatterPostProcessedchecks.Two consecutive
if (frontmatterPostProcessed)blocks set two flags; one block reads better.♻️ Proposed tidy-up
if (frontmatterPostProcessed) { canPlaceCursorAtCapture = false; + // Post-processing writes front matter of its own, so it counts as a + // change even when the capture body itself was a no-op. + rewroteAfterCompare = true; } - // Post-processing writes front matter of its own, so it counts as a - // change even when the capture body itself was a no-op. - if (frontmatterPostProcessed) rewroteAfterCompare = true;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/engine/CaptureChoiceEngine.ts` around lines 596 - 603, In the capture post-processing flow, combine the two consecutive frontmatterPostProcessed checks into a single conditional that sets both canPlaceCursorAtCapture and rewroteAfterCompare. Preserve the existing assignments and behavior when applyCapturePropertyVars returns a truthy value.
591-626: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
unchangedis reported after an unconditionalvault.modify.The canvas path (Lines 790-796) deliberately skips the write when the serialized text is identical, reasoning that rewriting identical bytes makes the reported
unchangedfalse. The markdown path doesn't: Line 586 always callsthis.app.vault.modify(file, newFileContent), so anunchangedoutcome still bumps mtime and wakes sync/watchers. Consider gating the write the same way for consistency.♻️ Sketch
- await this.app.vault.modify(file, newFileContent); + if (newFileContent !== priorContent) { + await this.app.vault.modify(file, newFileContent); + } contentCommitted = true;Note the interaction with
overwriteTemplaterOncebelow, which still needs to run on the file even when the capture body was a no-op.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/engine/CaptureChoiceEngine.ts` around lines 591 - 626, Gate the markdown-path vault write so this.app.vault.modify runs only when the serialized content actually changes, while preserving the existing unchanged outcome for identical content. Keep overwriteTemplaterOnce and related post-processing running even when the capture body is a no-op, and still write when that processing or other rewrites changes the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/src/content/docs/docs/Advanced/CLI.md`:
- Around line 128-158: Add the required release marker to the “Knowing whether
anything actually landed” section describing effect, using either an “Introduced
in QuickAdd X.Y.Z” line with the correct version or an “Available in the next
release” note. Keep the existing effect documentation unchanged.
In `@src/engine/CaptureChoiceEngine.selection.test.ts`:
- Line 1745: Update the onFileExists stub used by this test to return an
explicit priorContent value matching the expected comparison setup, alongside
file, newFileContent, and captureContent. Ensure the run() comparison can
produce “unchanged” when appropriate so the “changed” assertion no longer passes
vacuously.
---
Nitpick comments:
In `@src/engine/CaptureChoiceEngine.effect.test.ts`:
- Around line 80-82: Update the vi.mock declaration for InputSuggesterMock to
use the same relative module specifier style as the neighboring mocks, matching
the import path used by the engine so the intended module is reliably mocked.
In `@src/engine/CaptureChoiceEngine.ts`:
- Around line 596-603: In the capture post-processing flow, combine the two
consecutive frontmatterPostProcessed checks into a single conditional that sets
both canPlaceCursorAtCapture and rewroteAfterCompare. Preserve the existing
assignments and behavior when applyCapturePropertyVars returns a truthy value.
- Around line 591-626: Gate the markdown-path vault write so
this.app.vault.modify runs only when the serialized content actually changes,
while preserving the existing unchanged outcome for identical content. Keep
overwriteTemplaterOnce and related post-processing running even when the capture
body is a no-op, and still write when that processing or other rewrites changes
the file.
In `@src/engine/TemplateEngine.ts`:
- Around line 288-309: Clarify the attempt tracking in promptUntilAllowed by
replacing the post-decrement condition on remoteAttempts with an explicit,
readable check and decrement that preserves exactly MAX_REMOTE_FOLDER_ATTEMPTS
remote prompts before throwing ChoiceAbortError. Keep the existing
local-provider behavior and lastRejection fallback unchanged.
In `@src/engine/templateNoteDiscovery.test.ts`:
- Around line 25-27: Replace the local IN_APP_RUN declaration in
src/engine/templateNoteDiscovery.test.ts lines 25-27 with an import from a
shared test-fixture module. Apply the same change in
src/engine/templateNoteDiscovery.audit-template.test.ts lines 21-24, removing
each duplicated comment and sentinel while preserving the existing test usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6da5173c-f16d-44d9-bb65-514e5ef5dcc9
📒 Files selected for processing (25)
docs/src/content/docs/docs/Advanced/CLI.mdsrc/ai/tools/Agent.tssrc/cli/registerQuickAddCliHandlers.test.tssrc/cli/registerQuickAddCliHandlers.tssrc/engine/CaptureChoiceEngine.effect.test.tssrc/engine/CaptureChoiceEngine.notice.test.tssrc/engine/CaptureChoiceEngine.selection.test.tssrc/engine/CaptureChoiceEngine.tssrc/engine/MacroChoiceEngine.tssrc/engine/TemplateChoiceEngine.audit-template.test.tssrc/engine/TemplateChoiceEngine.discovery.test.tssrc/engine/TemplateChoiceEngine.notice.test.tssrc/engine/TemplateChoiceEngine.tssrc/engine/TemplateEngine.tssrc/engine/choiceOutcomeRecorder.test.tssrc/engine/choiceOutcomeRecorder.tssrc/engine/templateNoteDiscovery.audit-template.test.tssrc/engine/templateNoteDiscovery.test.tssrc/engine/templateNoteDiscovery.tssrc/interactive/engineChoice.tssrc/interactive/interactivePromptServer.tssrc/interactive/routePrompt.test.tssrc/interactive/routePrompt.tssrc/main.tssrc/types/ChoiceOutcome.ts
The stub omitted `priorContent`, so `newFileContent !== priorContent` was `"updated" !== undefined` and the `effect: "changed"` assertion could never have failed. Stating it means the test exercises the compare it claims to. Refs #1615
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.
Two ways a run lied to whoever was not sitting at the desktop: it opened prompts
nobody could answer, and it reported side effects it never had.
Closes #1614
Closes #1615
#1607 was in the same brief and is not in this diff - #1618 already closed it.
I verified that fix end to end against #1607's own repro before dropping it
(evidence).
#1614 - an interactive run opened its own prompts in Obsidian
quickadd:interactivepromises to forward a run's prompts to the connectedclient. It kept that promise for every prompt a script or the formatter opens,
because each of those hand-wrote an
if (choiceExecutor.promptProvider)branch atits call site. The engines never got one.
Before - a Template whose target note exists, run as
quickadd:interactive.The client polls and gets nothing; the chooser sits in the Obsidian window until
someone walks past the machine, on exactly the headless setups this seam exists
for.
POST /abortanswered{"interrupted":0}because it could not reach it./pollreturned nothingThe design
A seam, not a seventh hand-written branch - "remember to add the branch" had
already been missed six times.
routePrompt(executor, {remote, headless, app})owns only what is genuinely uniform: the order of the three destinations and
the
isCancellationError -> UserCancelErrormapping every site was repeating.Each site keeps its own modal call verbatim, and that is deliberate. A wrapper
that built the modal would have to model every per-site difference as an option
it understands, and the variance here is entirely in modal construction
(
renderItem,searchItems,valueExists,customValueLabel). The headlessbranch is not uniform either: most sites abort, but
MacroChoiceEnginelegitimately runs a sole exported member without asking, so a fixed three-step
ladder would have regressed every headless run of a single-member script. The
executor parameter is required, so an engine that cannot supply one is a
compile error rather than a prompt that silently opens on the desktop.
Engine replies are validated, which the in-app modal gets for free.
GenericSuggestercan only ever resolve an element of its own list, and routing aprompt must not widen a closed list into free text just because the answer now
arrives over HTTP.
RemotePromptProvider.suggesterreturns any string verbatimand maps a missing value to
""; both are right for the script callers it waswritten for and dangerous here -
""reachedgetFileExistsMode(""), whoseinternal
Unknown file exists mode:is what a client would have seen, and at thefolder chooser it created the note in the vault root where dismissing the modal
would have cancelled the run. The lenient path is untouched for the script and
formatter callers that depend on it.
Sites converted
Each was confirmed to strand a real run first - start
quickadd:interactiveinan isolated vault, then check both sides.
interactive === trueonly disabled the headless guardThree things the conversion forced:
displayis a fuzzy-search blob (basename + path +aliases) and the visible row comes from
renderItem, which does not travel. Itnow carries an explicit
title, so a client showsAreas/Work/Tom.mdratherthan
Tom Areas/Work/Tom.md Thomas.rejection. A client gets no Notice, so an identical re-prompt is
indistinguishable from a hang: a remote run is bounded at three attempts and
ends with the reason that actually fired.
Object.hasOwnbefore indexing the scriptmodule, since a reply that used to come from a fixed key list now arrives over
the wire.
The one I got wrong, and the adversarial pass that caught it
I originally left the capture-target pickers unconverted, reasoning that a
remote run forces the one-page preflight, which already collects the target as
__qa.captureTargetFilePath. Review attacked that claim; two shippedconfigurations break it, and both reproduce live:
Capture tocarrying format syntax (Journal/{{DATE:YYYY-MM}}/) -classifyCaptureTargetScopereturns null for any target containing{{…}}, sonothing is pre-collected.
shouldUseOnePageris falseeven for a remote run, so there is no preflight at all.
Both now route, with the confinement preserved rather than bypassed. The
folder-scoped picker needed nothing extra: every reply is re-prefixed into the
folder afterwards, which is exactly what
confinePreselectedToScopedoes. Thetag/property picker did - nothing downstream re-confines it, and in Obsidian the
rule is structural (
valueExistssuppresses the "Create new note" row for a namethat already exists, so a typed value can only ever create a new note). A
routed reply naming an existing note outside the scope is refused with that same
rule:
Two more things that had to land with it
Teardown errors are typed as aborts.
isCancellationErroris false for aplain
Error, so once engine prompts travel this bridge a client that simplystopped polling would be reported as a run failure - a red notice on a desktop
nobody is sitting at. Only load-bearing because of this change, hence in it.
A headless
quickadd:runno longer hangs on AI tool approval.Agent.confirmhad neither a provider route nor a non-interactive guard, so a Macro calling
quickAddApi.ai.agentwith a non-readOnly tool never returned at all - worse than#1614 rather than an instance of it. One-line guard here; routing it remotely is
#1631, because its four outcomes (
allow/allow-all/deny/abort, dismissal ==deny) fit neither existing wire prompt type.
#1615 - a run that changed nothing reported a side effect
successanswered "did the run finish?", which is not the question an automationasks - it asks "did anything land?". Those had come apart in two shipped
configurations that need no interactivity at all:
The Template case is the one the issue did not mention and is the more common: a
one-click setting whose whole purpose is to do nothing, reporting
verified:truewith a file path.
Why a new key instead of redefining
verifiedThe issue proposed surfacing this as
verified:false. I did not do that, andit is the one place I deliberately diverged.
verified:falseis already thepublished "not confirmed - go look" signal, and the handler's own comment invites
retry logic on it. Folding "we confirmed, and nothing happened" into the same value
makes a correct, permanently-unchanging run look retryable forever - a silent
break of a shipped contract, in the worst direction.
verifiedis frozen; the newfact got a new key.
unknownis stated, not omitted, on both legacy tails: a missing key reads asfalsetojq '.effect'and to!res.effectalike, which would turn "QuickAdd didnot look" into "nothing happened". The interactive legacy tail emitted neither flag
before, so a client could not tell it from the verified frame at all.
The claim is about persisted bytes
effectis required onChoiceOutcomeRecorder.success, with no default, so everycall site has to state its claim rather than inherit a positive one. That is what
surfaced the two Template sites: of the four success sites in the tree, the two
nobody had looked at were the two that commit nothing.
It is computed from the file, never the payload. A
!captureIsNoOppredicatefails immediately: create-if-not-found makes a real note (possibly with a rendered
template body) from an empty payload. A post-commit rewrite counts too - whole-file
Templater rewrites the note after the bytes were compared. And it is scoped to
the choice's target: append-link writes to a different note, and this flag does
not describe that.
The canvas path now checks the no-op before it writes. It re-serialises the
whole
.canvasJSON, so writing identical card text still rewrote the file - whichwould have made the
unchangedit was about to report false.Also reverted after review
I first exempted
unchangedfrom closing the outcome, reasoning it "left nothing aretry could duplicate". Review showed that is false - the run continues past the
commit point into the append-link step, which writes elsewhere - so letting a later
failure overwrite the success put the caller back to retrying a run that already
had side effects. Reverted;
successcloses the outcome as before.Validation
Every check below was run in this worktree's own isolated vault (Obsidian 1.13.0,
pnpm run obsidian:e2e), not just in tests.#1614
/poll;.modal-containeris[](screenshots above)sensible row titles
(
{{DATE}}in Capture to; one-page input never) reproduce before and route afterUnknown file exists mode:{"cancelled":true}->Execution cancelled by user, same as EscapePOST /abort->{"interrupted":1}where the issue documented0File 4exists -> "Increment" over the wire ->File 5.md#1615
unchanged,Inbox.mdbyte-identical (shasum before/after)doNothing->unchanged, byte-identicalchanged; new Template note ->createdSuite: 4734 passing, lint clean, typecheck clean. The nine existing assertions
that pinned the exact success object were updated to assert the expected
effectexplicitly rather than loosened to
objectContaining- all nine predictionswere right first time, which is itself a check on the classification.
Known limitations, stated rather than papered over
documented in
CLI.mdrather than left for a user to discover.report
donewhile its picker is open ([BUG] A Multi choice run from inside a macro is fire-and-forget, so the run reports done while its picker is still open #1630).effectcomes from the file-exists resolution, not a byte compare.Exact for
createdandunchanged;modifyExistingalways writes, sochangedcan in principle over-report a write whose bytes happened to match - the harmless
direction.
vault.modifywith identical content, touchingmtime. Pre-existing, unchanged here, and
effectnow tells the truth about it.currentLineand friends) reportschangedfrom the payload,not a byte compare: those branches mutate through the editor API, so honest
detection would mean reading the note before and after every editor capture. The
case [BUG] A capture that commits nothing is reported to automation as a verified success #1615 was filed about - an empty payload, where the insertion is skipped
entirely - is exact.
Review
Design reviewed adversarially before coding and again after; both passes are
reflected above. The second pass is why the capture-target pickers are in this
diff, why
unchangedcloses the outcome again, and whyeffectis scoped to thetarget rather than the vault.
Summary by CodeRabbit
New Features
effect(created/changed/unchanged) with clearer “verified” vs legacy behavior.Bug Fixes
Documentation