Skip to content

refactor(application): build model resources in one owner - #418

Merged
steipete merged 1 commit into
mainfrom
codex/model-resource-ownership
Sep 5, 2026
Merged

refactor(application): build model resources in one owner#418
steipete merged 1 commit into
mainfrom
codex/model-resource-ownership

Conversation

@steipete

@steipete steipete commented Sep 5, 2026

Copy link
Copy Markdown
Owner

One owner for model resources

The application previously constructed one model runtime through three factories, repeatedly declaring and forwarding the same environment, request options, and resolved run policy. Model selection now owns intent resolution; one model-runtime factory creates the metrics, provider bindings, executor, and executable model. Execution resources only compose the URL and asset flows.

The original environment, per-run environment, optional metrics environment, execution budgets, request-option precedence, CLI fallback policy, and stream/log/trace hooks retain their meaning. Both flows still share the same metric collectors and executor references. The old private constructor paths are deleted rather than kept as compatibility aliases.

This removes 98 production lines; expanded contract tests and documentation leave the complete change 56 lines smaller. Public exports and dependencies are unchanged.

Proof

  • Build and full project gate pass: 3,089 tests (43 skipped), 94.30% line coverage and 85.21% branch coverage.
  • All 40 application/architecture tests pass, including cycle detection and workspace/engine boundaries.
  • Expanded model-resource tests verify that selection allocates no runtime resources, construction happens once, reference identity is shared, overrides propagate, and explicit/default metrics environments stay distinct.
  • The built CLI preserves synthetic short-file output byte-for-byte in an isolated home with no inherited credentials.
  • Independent Codex review is clean for actionable P0–P2 findings.

The first build caught an overly permissive subprocess dependency type inherited from the lower-level executor. The final factory retains the original required subprocess function contract, and the complete build/gate/review were rerun successfully.

Hosted CI passes on the exact PR head: Node 24 gate, Chromium E2E, and Firefox smoke, with security checks green.

@clawsweeper

clawsweeper Bot commented Sep 5, 2026

Copy link
Copy Markdown

🦞👀
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.

@clawsweeper clawsweeper Bot added P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. 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. labels Sep 5, 2026
@clawsweeper

clawsweeper Bot commented Sep 5, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed September 5, 2026, 12:44 AM ET / 04:44 UTC.

ClawSweeper review

What this changes

Consolidates model-resource construction into one factory, keeps model selection separate, and updates contract tests and documentation.

Merge readiness

Ready for maintainer review

This remains a useful, focused refactor: main and v0.21.11 retain the intermediate factories. No actionable correctness or security defect was found, and repository policy protects this owner-authored PR from automatic closure.

Priority: P3
Reviewed head: 74c8535840e5dc317a1993f7eec52c4f4fbcb8fc

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, behavior-preserving refactor with expanded contract coverage and no actionable findings.
Proof confidence 🌊 off-meta tidepool Not applicable: The OWNER-authored internal refactor is exempt from contributor proof requirements and makes no material authority change. The reported built-CLI short-file smoke supports output preservation, while resource construction is covered by inspected contract tests.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The OWNER-authored internal refactor is exempt from contributor proof requirements and makes no material authority change. The reported built-CLI short-file smoke supports output preservation, while resource construction is covered by inspected contract tests.
Evidence reviewed 9 items Pinned change and merge identity: The checkout matches the supplied PR head. Raw commit records confirm its parent is the pinned main revision; the test merge has main followed by the exact PR head as parents and has the same tree as the head.
Factory preserves existing wiring: The consolidated factory preserves the original, per-run, and metrics environments; execution budgets; request-option precedence; provider bindings; and stream/log/trace hooks.
Shared flow resources remain intact: Execution resources construct the model once and pass its executor, metrics, and environment references into the existing URL/file context composition. Both production callers retain their previous arguments.
Findings None None.
Security None None.

How this fits together

The application layer turns CLI and daemon requests into shared execution resources for URL and file summaries. These resources connect model providers, streaming output, and usage metrics.

flowchart TD
  A[CLI or daemon request] --> B[Resolve model intent]
  G[Configuration and environment] --> B
  B --> C[Build model resources]
  G --> C
  C --> D[URL and file flows]
  D --> E[Provider or CLI model]
  E --> F[Summary output and metrics]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Code footprint Production −98 lines; tests +39; docs/changelog +3; total −56 The consolidation removes repeated production wiring while expanding resource-contract coverage.

Technical review

Best possible solution:

Keep one per-run model-resource owner while preserving shared executor, metrics, environment, and stream references across both flows.

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

Not applicable: this PR proposes an internal refactor rather than a reported runtime bug, and source comparison revealed no concrete introduced failure.

Is this the best way to solve the issue?

Yes. Combining the existing constructors removes forwarding layers without adding a competing API, changing configuration semantics, or altering flow ownership.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 49805c2c4356.

Labels

Label changes:

  • add P3: This is bounded internal cleanup that preserves existing user behavior and public exports.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The OWNER-authored internal refactor is exempt from contributor proof requirements and makes no material authority change. The reported built-CLI short-file smoke supports output preservation, while resource construction is covered by inspected contract tests.

Label justifications:

  • P3: This is bounded internal cleanup that preserves existing user behavior and public exports.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The OWNER-authored internal refactor is exempt from contributor proof requirements and makes no material authority change. The reported built-CLI short-file smoke supports output preservation, while resource construction is covered by inspected contract tests.

Evidence

What I checked:

  • Pinned change and merge identity: The checkout matches the supplied PR head. Raw commit records confirm its parent is the pinned main revision; the test merge has main followed by the exact PR head as parents and has the same tree as the head. (74c8535840e5)
  • Factory preserves existing wiring: The consolidated factory preserves the original, per-run, and metrics environments; execution budgets; request-option precedence; provider bindings; and stream/log/trace hooks. (src/application/model-runtime.ts:43, 74c8535840e5)
  • Shared flow resources remain intact: Execution resources construct the model once and pass its executor, metrics, and environment references into the existing URL/file context composition. Both production callers retain their previous arguments. (src/application/execution-resources.ts:196, 74c8535840e5)
  • Work remains distinct from main: Pinned main still constructs model resources through the separate runtime and executable-model factories; the proposed consolidation is not already present there. (src/application/model-runtime.ts:78, 49805c2c4356)
  • Latest release retains the previous structure: The v0.21.11 source also contains the separate factories, and its model-runtime and model-selection files have no differences from pinned main. (src/application/model-runtime.ts:78, 1dcbff752de7)
  • Focused contract coverage: The expanded test covers default and explicit metrics environments, no runtime allocation during selection, one-time construction, override propagation, and shared object identity. Existing flow-context tests cover the URL/file executor and environment references. (tests/application.model-runtime.test.ts:11, 74c8535840e5)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

@steipete
steipete merged commit 470af48 into main Sep 5, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant