Skip to content

docs: document auth profile failure policy contract#89613

Merged
steipete merged 795 commits into
mainfrom
codex/comment-contracts-after-88554
Jun 3, 2026
Merged

docs: document auth profile failure policy contract#89613
steipete merged 795 commits into
mainfrom
codex/comment-contracts-after-88554

Conversation

@steipete

@steipete steipete commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Continue the inline-comment pass after PR docs: clarify Android token comments #88554 on current main.
  • Document the auth-profile failure policy boundary that decides which failover reasons affect shared provider health.
  • Document heartbeat-only trigger policy prompt injection and the default non-heartbeat prompt shape.
  • Document embedded model fallback override precedence, including explicit empty override behavior.
  • Document RuntimePlan-first transcript policy resolution and legacy provider-runtime fallback adaptation.
  • Document embedded abort-settle cleanup timeout resolution and strict decimal env override behavior.
  • Document embedded attempt run decisions for write-lock timing, forwarded auth-profile provenance, unknown-tool guarding, hook gating, and tool-policy provider fallback.
  • Document the narrow replay-safe Codex app-server recovery retry gate for client-close and completion-idle failures.
  • Keep the change comment-only: no file header and no import-block comment.

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.

@steipete
steipete requested a review from a team as a code owner June 2, 2026 21:09
@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: XS maintainer Maintainer-authored PR labels Jun 2, 2026
@clawsweeper

clawsweeper Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed June 3, 2026, 4:55 PM ET / 20:55 UTC.

Summary
The PR adds inline comments and JSDoc contract notes across auth/profile, gateway, channel, agent, plugin, media, web, wizard, and utility surfaces without intending runtime behavior changes.

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.

  • Overbroad public retry comments: 2 comments. Two protocol comments advertise safe retry semantics that current runtime paths do not fully guarantee.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦞 diamond lobster
Patch quality: 🦪 silver shellfish
Result: blocked by patch quality or review findings.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Narrow the chat.send and node.invoke retry comments to current runtime guarantees.
  • Replace or enforce the pairing-detail sanitization wording.
  • Get human maintainer review for the remaining broad security-boundary wording.

Risk before merge

  • [P1] Public protocol retry comments can influence client behavior; overbroad safe-retry wording could cause duplicate user messages or side-effecting node commands after a lost response.
  • [P1] The PR is an XL comment-contract pass across auth, pairing, channel, plugin, and gateway surfaces, so even narrow fixes still need human maintainer review before merge.

Maintainer options:

  1. Narrow the inaccurate contracts (recommended)
    Update the chat.send, node.invoke, and pairing-detail comments so they describe current runtime guarantees without promising durable retry safety or display sanitization.
  2. Implement the stronger contract
    If maintainers want the comments to stand, add durable retry/dedupe and display-sanitization behavior with focused tests before keeping the stronger wording.
  3. Pause for owner review
    Because the PR is protected and very broad, maintainers can pause it for owner review instead of landing partial contract wording under uncertainty.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Narrow the public protocol comments for chat.send and node.invoke idempotency, and the pairing-detail sanitization wording, so they match current runtime behavior without changing runtime code.

Next step before merge

  • [P2] The concrete comment wording defects are narrow enough for an automated repair attempt, but the protected-label PR still needs maintainer review before merge.

Security
Needs attention: The diff has a concrete security-boundary wording issue around pairing detail sanitization, and broader security-facing comments need maintainer review before merge.

Review findings

  • [P1] Narrow the chat.send retry contract — packages/gateway-protocol/src/schema/logs-chat.ts:76
  • [P1] Narrow the node.invoke retry contract — packages/gateway-protocol/src/schema/nodes.ts:122
  • [P2] Avoid calling pairing details sanitized — packages/gateway-protocol/src/connect-error-details.ts:342
Review details

Best 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:

  • [P1] Narrow the chat.send retry contract — packages/gateway-protocol/src/schema/logs-chat.ts:76
    This comment promises safe retries after transport failures, but current chat.send only reuses cached or in-flight sends and does not provide a durable completed-send replay guarantee for every lost response. Please narrow the wording to the actual in-flight/cache behavior or add the stronger runtime contract first.
    Confidence: 0.9
  • [P1] Narrow the node.invoke retry contract — packages/gateway-protocol/src/schema/nodes.ts:122
    node.invoke currently creates a fresh request id and forwards another node.invoke.request; the node handlers execute incoming requests without a clear idempotency dedupe boundary. Calling retries safe can encourage clients to replay side-effecting node commands, so the comment should be narrowed unless the runtime contract is strengthened.
    Confidence: 0.9
  • [P2] Avoid calling pairing details sanitized — packages/gateway-protocol/src/connect-error-details.ts:342
    This helper trims and normalizes pairing fields, but it still copies device/role/scope strings that UI code later formats into user-facing error text. Please describe the output as normalized structured details, or sanitize every echoed display field before labeling this boundary sanitized.
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against a0717ef61c77.

Label changes

Label changes:

  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦞 diamond lobster and patch quality is 🦪 silver shellfish.
  • remove rating: 🧂 unranked krab: Current PR rating is rating: 🦪 silver shellfish, so this older rating label is no longer current.

Label justifications:

  • P2: This is a broad docs/comment contract PR with limited direct runtime blast radius but meaningful public protocol and security-boundary impact.
  • merge-risk: 🚨 message-delivery: The diff still documents message and node-command retry behavior more broadly than current delivery code guarantees.
  • merge-risk: 🚨 security-boundary: The diff labels pairing details as sanitized and broadly touches auth, credential, and snapshot safety contracts.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦞 diamond lobster and patch quality is 🦪 silver shellfish.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR body includes structured after-change terminal proof with whitespace checks and focused Vitest shards for the documented helper surfaces; no runtime behavior change is intended.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes structured after-change terminal proof with whitespace checks and focused Vitest shards for the documented helper surfaces; no runtime behavior change is intended.
Evidence reviewed

Security concerns:

  • [medium] Pairing details are labeled sanitized without display sanitization — packages/gateway-protocol/src/connect-error-details.ts:342
    buildPairingConnectErrorDetails copies normalized role, scope, and device strings, while UI formatting later includes role/scope values in user-facing text; the comment should not present this as a sanitization boundary unless the fields are actually display-sanitized.
    Confidence: 0.86

Acceptance criteria:

  • [P1] git diff --check.
  • [P1] git diff --check origin/main..HEAD.

What I checked:

Likely related people:

  • @obviyus: Current-main blame and log history for the gateway protocol schemas, chat send runtime, node registry, and pairing details point to commit 4f87400 across the central paths. (role: recent area contributor; confidence: high; commits: 4f8740029a0d; files: packages/gateway-protocol/src/schema/logs-chat.ts, packages/gateway-protocol/src/schema/nodes.ts, packages/gateway-protocol/src/connect-error-details.ts)
  • @vincentkoc: Recent current-main history on the chat send server path includes performance tracing work, making this a relevant adjacent owner for chat.send behavior review. (role: recent adjacent contributor; confidence: medium; commits: 98ff56d70e33; files: src/gateway/server-methods/chat.ts)
  • @steipete: The related merged documentation pass referenced by this PR was authored by the same contributor and touched nearby gateway client comment contracts. (role: adjacent documentation pass owner; confidence: medium; commits: 85beee613c64; files: packages/gateway-protocol/src/client-info.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jun 2, 2026
@steipete
steipete force-pushed the codex/comment-contracts-after-88554 branch 3 times, most recently from a9f0186 to 70cf356 Compare June 2, 2026 22:08
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels Jun 2, 2026
@steipete
steipete force-pushed the codex/comment-contracts-after-88554 branch from d76f940 to 70c0e45 Compare June 2, 2026 23:09
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jun 2, 2026
@steipete
steipete force-pushed the codex/comment-contracts-after-88554 branch from fd90766 to 0c9c751 Compare June 2, 2026 23:51
@steipete
steipete force-pushed the codex/comment-contracts-after-88554 branch 2 times, most recently from d836054 to 4fda299 Compare June 3, 2026 00:07

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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. */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/trajectory/export.ts
Comment on lines +934 to +935
// Public export API used by CLI/tests. The bundle is intentionally sanitized
// before writing so sharing it should not expose credentials or local paths.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +27 to +28
* Hooks can either enqueue wake events or spawn isolated agent turns; both paths
* sanitize external input before it reaches logs or system-event text.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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. */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

@steipete

steipete commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

Verification before landing:

Behavior addressed: inline comments and API docs added across gateway, plugin SDK, and CLI surfaces; no runtime behavior intended.
Real environment tested: local macOS checkout on d29fb5d plus GitHub PR checks.
Exact steps or command run after this patch: node scripts/run-vitest.mjs src/cli/program/route-args.test.ts src/cli/program/routes.test.ts src/cli/route.test.ts src/cli/parse-timeout.test.ts src/cli/cli-utils.test.ts src/cli/command-config-resolution.test.ts; git diff --check; git diff --check origin/main..HEAD; gh pr checks 89613 --watch=false.
Evidence after fix: focused local Vitest passed 2 shards, 6 files, 104 tests; both diff whitespace checks passed. GitHub Real behavior proof, workflow sanity no-tabs/actionlint, dependency guard, Socket checks, and early CI preflight/security-fast are green on the current head.
Observed result after fix: comments/docs-only PR remains mergeable; no local diff remains.
What was not tested: full CI and CodeQL were still in progress at the time of this landing note; no live product behavior was exercised because this is comments/docs-only source annotation work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents Agent runtime and tooling app: web-ui App: web-ui cli CLI command changes commands Command implementations gateway Gateway runtime maintainer Maintainer-authored PR merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: XL status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant