fix(err-06): return IpcError from every command that can emit a conflict code - #283
Open
entelecheia wants to merge 1 commit into
Open
fix(err-06): return IpcError from every command that can emit a conflict code#283entelecheia wants to merge 1 commit into
entelecheia wants to merge 1 commit into
Conversation
…ict code
Three commands reached a contract-code emitter and flattened its typed error
back to String with `.map_err(|e| e.to_string())`, so a conflict arrived at the
frontend as a plain string and `isTodayConflict` / the document_conflict branch
returned false:
- today_apply_plan_result (today_ai.rs) -> today_mutate, today_conflict
- task_calendar_set_sync (today_calendar.rs) -> today_mutate, today_conflict
- update_frontmatter_field (document.rs) -> assert_expected_revision,
document_conflict
No caller branched on them, so nothing regressed. The point is the boundary: it
had been drawn by what the frontend happened to branch on today, which gives a
future author no compile-time signal that the advertised recovery is missing.
Non-contract errors inside these commands ride From<String> unchanged, so their
text stays byte-identical. Tests that asserted a string prefix on a contract
code now assert err.code instead, which is what the contract was for.
`update_frontmatter_field`'s TypeScript wrapper called invoke directly with no
funnel. Now that the command rejects with a { code, message } object rather
than a string, a caller doing String(err) would have rendered "[object
Object]", so it routes through normalizeIpcError like saveDocument does.
Adds `no_code_emitting_path_flattens_its_error_to_string`: it scans the
emitter-calling modules for `map_err(|x| x.to_string())` and reports the
enclosing function. It matches only the closure's own argument, so
`map_err(|_| "literal".to_string())` (minting an error) and an incidental
`.to_string()` elsewhere on the line do not trip it. Proven by dropping the
allowlist and watching it name web_actions.rs:574 in apply_receipt().
apply_receipt is allowlisted with its reason: it is an internal helper, not a
command, and web_actions_apply consumes its Err as a retry-marker string that
never crosses IPC. Worth noting for later - web_actions.rs:860 treats a
today_conflict as skip-and-retry-next-run while :574 marks a task_conflict as
retry-needed. Same class of event, two treatments. Changing that is a behavior
change, so it is left alone here.
ERR-04's count moves 1128 -> 1124 (three commands plus one test helper), still
inside the [1118, 1138] band Phase 3 recorded.
![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: 607993ad25
ℹ️ 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".
Comment on lines
+124
to
+128
| const SOURCES: &[(&str, &str)] = &[ | ||
| ("web_actions.rs", include_str!("web_actions.rs")), | ||
| ("today_store.rs", include_str!("today_store.rs")), | ||
| ("today_calendar.rs", include_str!("today_calendar.rs")), | ||
| ("today_lifecycle.rs", include_str!("today_lifecycle.rs")), |
There was a problem hiding this comment.
When a new command calls an existing conflict-code emitter from any Rust module not manually listed in SOURCES, this test never examines that command, so it can still flatten the error to String while the purported ERR-06 regression guard passes. Because adding a caller does not require changing EMITTERS, there is no test or compiler signal reminding the author to extend this separate file list; derive the scanned sources/callers automatically or make the inventory exhaustive and self-validating.
Useful? React with 👍 / 👎.
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 ERR-06, one of the two follow-ups the Codex adversarial review raised on
Phase 3 (#279).
The gap
Three commands reached a contract-code emitter and flattened its typed error
back to
String:today_apply_plan_resulttoday_mutatetoday_conflicttask_calendar_set_synctoday_mutatetoday_conflictupdate_frontmatter_fieldassert_expected_revisiondocument_conflictA conflict on those paths arrived at the frontend as a plain string, so
isTodayConflictand thedocument_conflictbranch returned false.No caller branches on them, so nothing regressed and Phase 3's "display-only, no
frontend branch" justification was factually correct at the time. The point is
the boundary: it was drawn by what the frontend happened to branch on today,
which gives a future author no compile-time signal that the advertised recovery
is unavailable there.
The guard
no_code_emitting_path_flattens_its_error_to_stringscans the emitter-callingmodules for
map_err(|x| x.to_string())and names the enclosing function. Itmatches only the closure's own argument, so
map_err(|_| "literal".to_string())(minting a fresh error) and an incidental
.to_string()elsewhere on the line donot trip it - the first draft did, and reported four false positives.
Proven rather than assumed: dropping the allowlist makes it report
web_actions.rs:574 in apply_receipt() flattens task_transition(.No
IpcResult<T>alias was added. The inventory test covers the intent withoutintroducing a second way to spell the return type.
Deliberately out of scope
apply_receiptis allowlisted with its reason in the test: it is an internalhelper, not a command, and
web_actions_applyconsumes itsErras the reasonstring on a retry marker, so the value never crosses IPC and no frontend branch
can read a code.
Worth flagging separately:
web_actions.rs:860treats atoday_conflictasskip-and-retry-next-run, while
:574marks atask_conflictas retry-needed.Same class of event, two treatments, in one file. Reconciling them is a behavior
change, so it is left alone here.
Verification
cargo test --lib1220 passed / 0 failed,clippy -D warningsandfmt --checkclean,
pnpm typecheckexit 0, vitest 1392 passed, eslint clean on the touchedfile.
Non-contract errors inside these commands ride
From<String>unchanged, so theirtext stays byte-identical; tests that asserted a string prefix on a contract
code now assert
err.code, which is what the contract was for.update_frontmatter_field's TypeScript wrapper calledinvokedirectly with nofunnel. Now that the command rejects with an object rather than a string, a
caller doing
String(err)would have rendered[object Object], so it routesthrough
normalizeIpcErrorlikesaveDocumentdoes.ERR-04's count moves 1128 -> 1124 (three commands plus one test helper), still
inside the
[1118, 1138]band Phase 3 recorded.ERR-05 is untouched and stays open.