Skip to content

Propagate source conditions to review child processes - #398

Merged
firejune merged 2 commits into
mainfrom
jinni/385-source-child-resolution
Nov 10, 2026
Merged

firejune merged 2 commits into
mainfrom
jinni/385-source-child-resolution

Conversation

@firejune

Copy link
Copy Markdown
Contributor

Closes #385

Changes

  • Reuse jobRuntimeExecArgv across all five review-act fetch children and the Review CLI child. Propagate only the supported source condition and preserve default resolution.
  • Exercise the six actual child launchers under both source-flag syntaxes and default mode, asserting child execArgv, selected Review/Play entries, entry evaluation markers and recorded writer outcomes.
  • Preserve the existing source-CI child observer.

Verification

Nova ordinary pool ran the eight affected test files, typecheck and lint at the exact changed-file hashes. Result was 162 pass, 2 existing source-only skips, 0 fail and 1340 assertions. The job held a slot for 43.28 seconds and ended quiescent.

The isolated package fixture instruments copied Play entries. When registry packaging lacks its source entry, the existing fixture fallback is a source entry that re-exports dist. These checks establish actual child condition propagation and selected-entry evaluation. Hosted source-linked CI supplies the sibling-source resolution evidence.

An initial new-test expectation that the HTTP fetch adapter would leave Play unevaluated failed. The observed static import graph evaluates the marked entry, and the corrected assertion is retained with both raw runs.

Parent independent diff and input-hash review completed. Hosted CI and the single automatic review remain pending.

Reuse the supported runtime flag filter across six launchers and verify actual child entry resolution and writer outcomes in source and default modes.

Co-Authored-By: Jinni <342598851+fj-jinni[bot]@users.noreply.github.com>
Co-Authored-By: Naru <342599188+fj-naru[bot]@users.noreply.github.com>
@firejune firejune added the by:codex Written by a Codex (Naru) worker; Claude side judges the review label Nov 10, 2026
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-10T14:20:52.324455Z eb3ef18 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@firejune

Copy link
Copy Markdown
Contributor Author

Claude review (cross-family)

Reviewer: Claude (HQ). Read: src/job-writer.ts, src/choice-writers.ts, src/feedback.ts, src/parts-review.ts, src/review-read.ts, src/signoff-checkpoint-act.ts, src/signoff.ts, src/cli.ts, src/init.ts, src/collection.ts, scripts/run-source-ci.ts, tests/pins.test.ts, AGENTS.md at head eb3ef18bae6fccad95ff4dff013e8c2067604a95. Verdict: changes needed

What I checked

  • Launcher coverage: I grepped every Bun.spawn / Bun.spawnSync / process.execPath site in src/. The six launchers named in Propagate source resolution to review-act and parts CLI children #385 now pass jobRuntimeExecArgv(). cli.ts and job-writer.ts already go through jobRuntimeCommand. The rest are git and bun install, which are not child CLIs. No launcher is missed.
  • Flag leakage: jobRuntimeExecArgv returns only [SOURCE_CONDITION_FLAG] or [], so inspector ports, --eval, preload and test-runner flags cannot reach the child. The test confirms that --smol and --conditions=other are dropped.
  • Installed mode: the filter returns [], so each argv is byte-identical to main. Behaviour does not change.
  • Argv order: in every launcher the runtime flag comes before the script path, and the Review CLI args stay after the entry.
  • Test: I ran it locally at this head and read the hosted source-linked log.

Findings

  • P1: In the hosted source-linked job, the new test inherits the outer source-CI observer: it keeps BUN_OPTIONS and points RIG_CREATE_CHILD_OBSERVER_DIR at its own obsDir. Each child therefore loads both preloads and writes two observation files, so takeSingleObs throws and the required source-linked check is red at this head (parts-review-act, source-equals, exit 1). Strip the outer preload from BUN_OPTIONS, or give the test's observer its own env names. tests/pins.test.ts:632-641
  • P2: The new test has no explicit timeout. It needed 11.65 s locally, so it fails under the default 5 s bun test timeout; the other multi-child tests in this suite set 30-180 s. tests/pins.test.ts:732
  • P3: The Play marker cannot detect a mixed load. The source entry assigns = after the dist entry's ??=, so loadedPlay reads the source path even when a child also evaluated dist directly. The PR body discloses this limit for the dist re-export fallback. tests/pins.test.ts:65

The production change looks correct; once the P1 test collision is fixed and CI is green, this is an approve.

Keep hosted source CI observer inputs intact and cover inherited preload coexistence with a bounded test timeout.

Co-Authored-By: Naru <342599188+fj-naru[bot]@users.noreply.github.com>
Co-Authored-By: Jinni <342598851+fj-jinni[bot]@users.noreply.github.com>
@firejune

Copy link
Copy Markdown
Contributor Author

Claude review (cross-family): readback at 1a39c6ed4587e22108b70ad3a7e5889d5c8ceca1

Scope: the repair commit eb3ef18..1a39c6e touches only tests/pins.test.ts. The production diff against main is unchanged from the reviewed eb3ef18.

Earlier finding Status at this head
P1: the fixture observer reused the RIG_CREATE_CHILD_OBSERVER_* namespace, so in hosted source-linked CI the outer observer and the fixture both recorded children Fixed. The fixture now uses its own RIG_PINS_TEST_* namespace and tags its records observer: 'pins-test-fixture'. The reader rejects untagged records. The fixture also strips the outer preload from execArgv. When a hosted observer is present, the test leaves the CI env alone and only appends its own preload.
P2: the test had no timeout (11.65 s on the default) Fixed. 60_000 timeout.

New coverage: when the test runs without a hosted observer, it installs a benign outer observer shaped like scripts/run-source-ci.ts. It then asserts that 6 outer records exist (5 fetch, 1 CLI), that none are errors, and that each has the expected resolvedPlay. That gives the inherited-preload coexistence a regression test.

Cross-check against scripts/run-source-ci.ts:204-214: the hosted observer filters on its own expected entries. The fixture's children run from a tmp linked workspace with different entry paths, so they produce no hosted kind records. They cannot inflate or satisfy the CI's own count.

Verdict: the change is acceptable. Signing requires hosted check and source-linked green at this exact head; both are pending at the time of this readback.

@firejune

Copy link
Copy Markdown
Contributor Author

Naru integration sign at 1a39c6ed4587e22108b70ad3a7e5889d5c8ceca1. Read the six production launcher edits and test-only repair. Exact-head hosted check and source-linked are SUCCESS in run 42836472648. Claude cross-family readback is recorded above, single automatic AI review completed at the original production head. Nova inherited-observer regression plus type/lint passed. The Play entry marker retains the disclosed mixed-load limitation. Ready to merge.

@firejune
firejune merged commit 5d6bd60 into main Nov 10, 2026
2 checks passed
@firejune
firejune deleted the jinni/385-source-child-resolution branch November 10, 2026 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

by:codex Written by a Codex (Naru) worker; Claude side judges the review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Propagate source resolution to review-act and parts CLI children

1 participant