docs: document auth profile failure policy contract#89613
Conversation
|
Codex review: needs changes before merge. Reviewed June 3, 2026, 4:55 PM ET / 20:55 UTC. Summary Reproducibility: yes. for the review findings: the latest PR head contains the overbroad comments, and current-main source shows the corresponding runtime and UI paths have narrower guarantees. Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Security Review findings
Review detailsBest possible solution: Narrow the retry and pairing-detail comments to the exact current guarantees, then have a maintainer review the remaining broad security and delivery contract wording before merge. Do we have a high-confidence way to reproduce the issue? Yes for the review findings: the latest PR head contains the overbroad comments, and current-main source shows the corresponding runtime and UI paths have narrower guarantees. Is this the best way to solve the issue? No; comment-only contract documentation is a maintainable approach only when it matches current behavior, and these public comments currently overstate retry and sanitization guarantees. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against a0717ef61c77. Label changesLabel changes:
Label justifications:
Evidence reviewedSecurity concerns:
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
a9f0186 to
70cf356
Compare
d76f940 to
70c0e45
Compare
fd90766 to
0c9c751
Compare
d836054 to
4fda299
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 188d6d64a6
ℹ️ 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".
| chunks: AgentMessage[][]; | ||
| }; | ||
|
|
||
| /** Messages safe to summarize plus notes for messages too large to fit in a summary request. */ |
There was a problem hiding this comment.
Do not label oversized fallback messages summary-safe
When callers use the exported buildOversizedFallbackPlan() directly, smallMessages are the original AgentMessage objects after only a size check (smallMessages.push(msg)), not the result of sanitizeCompactionMessages(), which is the boundary that strips toolResult.details and runtime-context transcript entries before summaries. This new type comment says those messages are safe to summarize, so a future caller can feed them directly to a summary prompt and leak runtime-only/tool-result details; either sanitize at plan construction or narrow the doc to size-safe only.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3acf1ec34
ℹ️ 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".
| // Public export API used by CLI/tests. The bundle is intentionally sanitized | ||
| // before writing so sharing it should not expose credentials or local paths. |
There was a problem hiding this comment.
Avoid promising path-free trajectory exports
When transcript/tool text contains an embedded absolute path outside the workspace, HOME/USERPROFILE, or state dir (for example failed reading /tmp/openclaw-debug/foo.log), the exporter leaves it intact: maybeRedactPathString() only redacts whole-string absolute paths or strings containing known prefixes, and the later bundle-file pass uses tool-payload redaction rather than arbitrary local-path redaction. This new comment says shared bundles should not expose local paths, which can lead support tooling or callers to treat trajectory exports as path-free when they are only best-effort redacted for known locations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86c20e9e86
ℹ️ 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".
| * Hooks can either enqueue wake events or spawn isolated agent turns; both paths | ||
| * sanitize external input before it reaches logs or system-event text. |
There was a problem hiding this comment.
Do not claim hook payloads are sanitized
For the /hooks/wake path, external text is only trimmed by normalizeWakePayload() and then dispatchWakeHook() enqueues value.text directly as a system event, so inputs such as line-leading System: or bracketed internal tags are not passed through sanitizeInboundSystemTags(). This comment can lead future hook/log/system-event callers to rely on a sanitization boundary that the wake path does not actually provide.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35e0df0f2c
ℹ️ 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".
| export function resolveMemoryHostEventLogPath(workspaceDir: string): string { | ||
| return path.join(workspaceDir, MEMORY_HOST_EVENT_LOG_RELATIVE_PATH); | ||
| } | ||
|
|
||
| /** Append one memory host event, creating the dreams directory with symlink-safe writes. */ |
There was a problem hiding this comment.
Do not call event-log mkdir symlink-safe
When workspaceDir contains a symlinked memory parent, the plain fs.mkdir(path.dirname(eventLogPath), { recursive: true }) follows that symlink and can create .dreams outside the workspace before appendRegularFile(..., rejectSymlinkParents: true) rejects the later append. This new contract can lead future callers to rely on this helper for symlink-safe directory creation in untrusted workspaces; either create the directory through the fs-safe boundary too, or narrow the comment to the append step only.
Useful? React with 👍 / 👎.
|
Verification before landing: Behavior addressed: inline comments and API docs added across gateway, plugin SDK, and CLI surfaces; no runtime behavior intended. |
Summary
main.Verification
Behavior addressed: comment-only follow-up for auth-profile failover health policy, heartbeat trigger prompt policy, embedded model fallback override policy, attempt transcript policy selection, abort-settle timeout resolution, embedded run-decision contracts, and Codex app-server recovery retry policy; no runtime behavior intended.
Real environment tested: local source checkout.
Exact steps or command run after this patch: git diff --check; git diff --check origin/main..HEAD; pnpm test src/agents/embedded-agent-runner/run/auth-profile-failure-policy.test.ts; pnpm test src/agents/embedded-agent-runner/run/trigger-policy.test.ts; pnpm test src/agents/embedded-agent-runner/run/fallbacks.test.ts; pnpm test src/agents/embedded-agent-runner/run/attempt.transcript-policy.test.ts; pnpm test src/agents/embedded-agent-runner/run/attempt.abort-settle-timeout.test.ts; pnpm test src/agents/embedded-agent-runner/run/attempt.run-decisions.test.ts; pnpm test src/agents/embedded-agent-runner/run.codex-app-server-recovery.test.ts.
Evidence after fix: whitespace checks passed; focused Vitest shards passed with 6 auth-profile failure-policy tests, 7 trigger-policy tests, 3 fallback tests, 3 transcript-policy tests, 5 abort-settle-timeout tests, 9 run-decision tests, and 10 Codex app-server recovery tests.
Observed result after fix: PR branch pushed from current main after confirming PR #88554 is merged into main.
What was not tested: broader agent/gateway suites, because this PR only adds declaration documentation for tested helpers.