Session save failures now carry an errno and a photographable read-out - #186
Conversation
write_atomic returned a bare bool and threw away every errno; a total fallback failure logged only "could not persist to ANY candidate path", indistinguishable from a server-side auth problem for a reporter with no shell. write_atomic and create_private_temp now return a typed WriteFailure carrying the OS errno where there is one, and save_legacy_fallback_locked logs per candidate: path, parent directory uid/gid/mode, and the errno or refusal class — iterating whatever paths::session_candidates() returns rather than assuming a fixed count. The failure read-out's existing Details card (screens::login::support_line) now also names the persistence class, key-manager stage and service error code already defined on telemetry::incident::IncidentContext but previously only sent to Sentry, never shown. Both projections read the same typed evidence through one new storage_evidence_line helper; an absent field renders as a fixed "unknown" class rather than disappearing, so the line's shape can never signal more than it knows. Reachable on every build, behind the affordance that already exists today, with no devtriggers or debug-flavour gate. PRIVACY.md and the in-app Privacy Policy named every sign-in-report field event_body could emit except these three, already shipping silently; their prose is corrected to match what the serializer has sent all along. No consent-scope change, no POLICY_VERSION bump. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…the earlier refusals too save_legacy_fallback_locked accumulated a CandidateDiagnostic per refused candidate but only logged it on the total-failure paths; both success arms returned early and threw the evidence away. Since 4f1c787 added in_runtime_dir("auth.json") as a strictly-last candidate, the common case on a jail like /media/developer is exactly this shape — durable candidates refuse, the runtime-dir one succeeds — so the errno/path/parent-mode evidence that explains WHY the durable ones refused was being discarded on the one path that shape actually takes. Both success arms now log the accumulated failures, with wording that says the save succeeded via a later candidate rather than reusing the total-failure "refused" phrasing. Silent when failures is empty, exactly as before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Codex review, one round, on the combined result (this branch sits on 4f1c787). One blocking finding, now fixed in 00aad70. Both success paths discarded the accumulated diagnostics. Both paths now log the refusals before returning, through the same No finding on the other four: nothing secret reaches the log or the screen (paths, numeric ownership, errno — no token, server identity or username); the three screen fields were already serialized by One factual note for the record, not a change here: |
![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: 00aad70e30
ℹ️ 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".
| LinkClass::Unknown => offer.key.kind.code().to_string(), | ||
| link => format!("{}.{}", offer.key.kind.code(), link.code()), | ||
| }; | ||
| let storage = crate::telemetry::incident::storage_evidence_line(offer.context.as_ref()); |
There was a problem hiding this comment.
For an actual persistence failure, this call always receives a context whose storage fields are unset: repository-wide, with_persistence is only called by tests, remains #[allow(dead_code)], and SessionMachine::apply_persistence_completion creates only a PersistenceWarning while discarding the failed outcome. Consequently production support lines always append persistence:unknown keymgr:unknown svc:unknown, and the newly documented report fields are never emitted. Wire the completion outcome into a retained SaveFailed incident before projecting it here.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
| LinkClass::Unknown => offer.key.kind.code().to_string(), | ||
| link => format!("{}.{}", offer.key.kind.code(), link.code()), | ||
| }; | ||
| let storage = crate::telemetry::incident::storage_evidence_line(offer.context.as_ref()); |
There was a problem hiding this comment.
When a fresh session save actually fails, the owner sets persistence_warning, but details_offered() unconditionally returns false while that warning exists. Since this added storage evidence is rendered only inside the Details card, the locked-out user in the motivating no-shell scenario sees only the Continue-unsaved control and cannot photograph any of these values; make the warning surface provide access to the diagnostic read-out.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
A save failure told a reporter with no shell exactly nothing:
write_atomicreturned a barebooland discarded every errno, so a total fallback failure logged only
could not persist to ANY candidate path— indistinguishable from a server-side authorizationproblem, and useless for deciding whether the jail refused the write or the path was never there.
write_atomicandcreate_private_tempnow return a typedWriteFailurecarrying the OS errnowhere there is one, and
save_legacy_fallback_lockedlogs one line per failed candidate: the path,the parent directory's uid/gid/mode, and the errno or refusal class. It iterates whatever
paths::session_candidates()returns rather than assuming a fixed count, so it keeps telling thetruth as that list changes. This is the difference between EACCES and EROFS being visible at all —
on the reporting set they mean two entirely different things, one a permission bit and one a
read-only mount.
The failure read-out's existing Details card also names the persistence class, key-manager stage
and service error code. Those three were already defined on
telemetry::incident::IncidentContextand already sent to Sentry; they were simply never shown to the person looking at the screen. Both
projections now read the same typed evidence through one
storage_evidence_linehelper, and anabsent field renders as a fixed
unknownrather than vanishing, so the line's shape can neversignal more than it knows. No new visual primitive, no devtrigger, no debug-flavour gate — it sits
behind the affordance that already ships.
PRIVACY.mdand the in-app privacy policy named every sign-in-report fieldevent_bodycould emitexcept those three, which have been shipping silently. Their prose is corrected to match what the
serializer has always sent. That is a notice correction, not a widening: no consent scope changes
and
POLICY_VERSIONdoes not move.Known gap, stated rather than papered over:
details_offered()returnsfalsewhile apersistence_warningis up, which is precisely the reporter's case — so the on-screen half doesnot reach the failure that prompted this. The logged per-candidate evidence does. Lifting that
suppression is a separate change.
🤖 Generated with Claude Code