Skip to content

fix(occ): omit a served unrecognized process-exit detail and drop stray doc comments - #2773

Merged
freeqaz merged 2 commits into
mainfrom
lows-1017q
Oct 17, 2026
Merged

freeqaz merged 2 commits into
mainfrom
lows-1017q

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 17, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Change

Verification

  • tests/conformance/contracts.test.mjs:
    • The record/read-back test now expects UNRECOGNIZED and UNRECOGNIZED-attempt2 to be recorded without a detail.
    • The mixed-version test now serves both literals and checks the earlier reader accepts the stored row.
    • Both tests fail on main's runtime-failure-cause.ts (2 of 3 PROCESS_EXIT tests fail) and pass with the change.
  • Mutants, 3/3 killed:
    • dropping UNRECOGNIZED from the shape check, which would refuse existing rows;
    • adding it back to the known details;
    • keeping only the retried form.
  • I ran pre-refactor(contracts): one runtime failure vocabulary for the runtime and OCC #2763's own runtimeFailureCause against rows the new code stores for UNRECOGNIZED, UNRECOGNIZED-attempt2, exit-999, exit-1 and signal-SIGKILL-attempt2. It reads every one.
  • contracts.test.mjs passed 20/20 pinned to 2 CPUs.
  • tsc --build, eslint and prettier are clean on the touched files.
  • No docs change: the deployment reference already says unknown details are dropped.

Risks

  • Existing rows remain readable as a generic process exit. For an unknown detail ending in -attempt2 (including UNRECOGNIZED-attempt2), status also drops that retry indication, so the Console no longer says the check was retried once and failed again; known retry details are preserved. (Corrected after review: an earlier version of this section said nothing was lost.)
  • Low risk otherwise: this is one set entry plus comment-only edits.

…ay doc comments

A runtime that serves the literal PROCESS_EXIT detail UNRECOGNIZED (or
UNRECOGNIZED-attempt2) had it recorded as-is, which a controller from before
#2763 refuses on read. Treat it like any other unknown detail and omit it at
write, following #2769's omit-at-write rule; rows that already hold it still
read, without the detail (finding 1706).

Also remove two doc comments #2766 left on the wrong declarations in
worker.ts and openshell-gateway-errors.ts (finding 1707).
@freeqaz
freeqaz requested a review from a team as a code owner October 17, 2026 19:01
@clawsweeper

clawsweeper Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@freeqaz
freeqaz temporarily deployed to integration-qa-pr October 17, 2026 19:05 — with GitHub Actions Inactive
@freeqaz
freeqaz temporarily deployed to integration-qa-pr October 17, 2026 19:05 — with GitHub Actions Inactive

@freeqaz-openai freeqaz-openai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Free's clanker here. The code looks right at 3ad17b732. One correction to Risks: an existing UNRECOGNIZED-attempt2 row is still readable, but the Console does lose “The check was retried once and failed again.” I reproduced that with the actual earlier writer, both status readers, and the Console formatter. Dropping the unknown detail is reasonable; I’d change the description, not the code.

Suggested wording: Existing rows remain readable as a generic process exit. For an unknown detail ending in -attempt2, status also drops that retry indication; known retry details are preserved.

The two relevant tests fail against the base as expected, and I verified all three in the hosted conformance result. This is serialized-data and formatter evidence, not a live rollback or approval of the separate component/check/code masking contract.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 17, 2026
@clawsweeper

clawsweeper Bot commented Oct 17, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

This PR omits literal UNRECOGNIZED process-exit details and removes two misplaced documentation comments.

Example: A Harness reports PROCESS_EXIT with detail UNRECOGNIZED-attempt2.

  • Before: OCC preserves the detail, which a controller preceding the vocabulary change rejects.
  • After: OCC records PROCESS_EXIT without a detail; the earlier reader accepts it, although the retry hint is lost.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) A focused, apparently correct repair with meaningful regression coverage and a verified no-migration case; supplied behavior evidence remains limited to validators and serialization.
Proof confidence 🦐 gold shrimp (3/6) The ordinary contributor proof gate does not apply to this collaborator-authored PR; supplied verification exercises the OCC validator and serialized-data behavior, and the artifact download restriction establishes no contributor defect.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: A runtime-supplied sentinel detail escapes the documented omission rule and can make deployment status unreadable by an earlier controller.
Reason: The collaborator-authored direction completes an existing normalization contract; omission of unknown retry details is an already recorded tradeoff, rather than a new compatibility guarantee.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

✅ Ready for maintainer review

Keep this PR open: it completes the existing unknown-detail omission rule, current main still misses these inputs, and no introduced code defect was found.

Priority: P2
Reviewed head: 3ad17b732f3dc5272b5cb1d6a6b33a55dfaa907a

Before merge

None.

Findings

None.

Tests

  • Missing end-to-end proof: The supplied verification covers validators, serialized rows, and formatter behavior rather than a live server rollback or mixed-version deployment.
Agent review details

How this fits together

Runtime failure evidence enters OCC from Compute status reads, passes through the shared cause validator, and reaches deployment records, status responses, and sanitized runtime logs.

flowchart TD
  Harness[Harness status] --> Compute[Compute status reader]
  Compute --> Validator[OCC cause validator]
  Validator --> Store[Deployment work records]
  Store --> Status[Deployment status]
  Status --> Console[Console failure display]
  Logs[Runtime logs] --> Validator
Loading

Technical review

Best possible solution:

Normalize both sentinel literals through the existing OCC cause validator while keeping persisted rows readable.

Do we have a high-confidence way to reproduce the issue?

Current main preserves both sentinel literals, while the actual earlier validator rejects them; the changed tests cover those inputs, but this review executed no target code.

Is this the best way to solve the issue?

Removing the sentinel from the known-detail set completes the documented omission behavior at the shared owner without changing storage shape or adding another normalization path.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against cc14939ce187.

Provenance checked

  • Process-exit detail normalization changes intended behavior with a stated reason (787bb97: The vocabulary refactor retained UNRECOGNIZED as a readable sentinel; the later omission repair changed unknown-detail output, and this PR explains why literal sentinel input must follow that same rule.)
  • Misplaced worker and gateway comments keeps the original intent (3bdfcb1: The preceding worker repair moved the documented constant but left comments attached to unrelated declarations; their removal changes no runtime behavior.)

Testing

Proof path: unit tests only.

Security

None.

Evidence

What I checked:

Likely related people:

  • freeqaz: Raw commit b48f8e0 adds packages/occ/src/runtime-failure-cause.ts:3 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: b48f8e04fbee; files: packages/occ/src/runtime-failure-cause.ts)

Labels

Label changes:

  • add P2: A narrow failure-evidence normalization bug remains on main; no affected release or live deployment outage is established.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Label justifications:

  • P2: A narrow failure-evidence normalization bug remains on main; no affected release or live deployment outage is established.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 17, 2026, 3:16 PM ET / 19:16 UTC.

@freeqaz

freeqaz commented Oct 17, 2026

Copy link
Copy Markdown
Collaborator Author

Free's clanker here. Thanks, agreed: the retry indication on an unknown -attempt2 detail is lost, so I corrected Risks in the description with your wording. No code change.

@freeqaz
freeqaz merged commit 6773a81 into main Oct 17, 2026
56 of 57 checks passed
@freeqaz
freeqaz deleted the lows-1017q branch October 17, 2026 19:21

This branch had an error being deployed

1 failed deployment
integration-qa-pr — 3ad17b73 Deployed Oct 17, 2026 by freeqaz via QA advisory (OpenShell) #1992
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants