fix(greenfield): seal candidates before review - #111
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds candidate-sealing automation, external evidence-envelope requirements, read-only reviewer procedures, lifecycle updates, repository script wiring, and comprehensive sealing tests. ChangesCandidate sealing and evidence workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Owner
participant SealScript
participant VerificationSubject
participant EvidenceEnvelope
Owner->>SealScript: Provide candidate, base, output, and commands
SealScript->>VerificationSubject: Create detached candidate clone
VerificationSubject->>VerificationSubject: Run diff check and verification commands
SealScript->>VerificationSubject: Inspect commit, tree, and cleanliness state
SealScript->>EvidenceEnvelope: Write observations and seal validity
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e994015cc
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
scripts/seal-candidate.mjs (1)
31-41: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd timeouts to
spawnSyncinvocations.None of the
git,gitOutput, orrunCommandspawnSynccalls set atimeout. A hunggitoperation or, more importantly, a hung caller-supplied verification command (Line 126-131) will block the seal process indefinitely with no recovery, defeating the goal of a reliable, automatable evidence-generation step run as part ofpnpm check.♻️ Suggested fix
function git(repository, ...args) { - const result = spawnSync('git', args, { cwd: repository, encoding: 'utf8' }); + const result = spawnSync('git', args, { cwd: repository, encoding: 'utf8', timeout: 30_000 }); if (result.status !== 0) fail(`git ${args.join(' ')} failed: ${result.stderr.trim()}`); return result.stdout.trim(); }result = spawnSync(command, { cwd: repository, encoding: 'utf8', shell: true, + timeout: <appropriate-bound>, stdio: ['ignore', logFile, logFile], });Also applies to: 121-145
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/seal-candidate.mjs` around lines 31 - 41, Configure a finite timeout on every spawnSync invocation used by git, gitOutput, and runCommand, including caller-supplied verification commands. Use the script’s existing timeout configuration if available, ensure timed-out processes are detected and reported through the existing failure path, and preserve current command output handling for successful executions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/delivery/greenfield/delivery-policy.md`:
- Around line 85-89: Make the observed target base ref a first-class seal field,
pairing its exact ref with the resolved base commit and tree throughout the
sealing contract. Update Rule 11 and lifecycle step 5 in
docs/delivery/greenfield/delivery-policy.md, the external seal envelope in
docs/delivery/greenfield/reviewer/README.md, required seal fields in
docs/delivery/greenfield/reviewer/review-checklist.md, owner verification
evidence in docs/delivery/greenfield/story-contract.md, and mandatory
verification evidence in docs/delivery/greenfield/verification.md.
In `@scripts/seal-candidate.mjs`:
- Around line 202-237: Update the catch handling around the verification loop in
the candidate-sealing flow so every command after the last recorded observation
receives a skipped entry when a mid-loop exception occurs. Preserve existing
observations and their order, use the existing “verification subject setup
failed” reason for unattempted commands, and ensure the envelope still contains
one command record per command even when observations is partially populated.
In `@scripts/seal-candidate.test.mjs`:
- Around line 205-214: Update the test around seal to pass an options object
with withPnpmSeparator enabled instead of the primitive true, ensuring the
command exercises the pnpm forwarded -- separator path and still asserts
successful execution.
---
Nitpick comments:
In `@scripts/seal-candidate.mjs`:
- Around line 31-41: Configure a finite timeout on every spawnSync invocation
used by git, gitOutput, and runCommand, including caller-supplied verification
commands. Use the script’s existing timeout configuration if available, ensure
timed-out processes are detected and reported through the existing failure path,
and preserve current command output handling for successful executions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c6afb26-bc7e-4511-8a70-dd232ee00848
📒 Files selected for processing (10)
docs/delivery/greenfield/delivery-policy.mddocs/delivery/greenfield/reviewer/README.mddocs/delivery/greenfield/reviewer/review-checklist.mddocs/delivery/greenfield/story-contract.mddocs/delivery/greenfield/verification.mdpackage.jsonscripts/check-active-repository.mjsscripts/check-delivery-track.test.mjsscripts/seal-candidate.mjsscripts/seal-candidate.test.mjs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/delivery/greenfield/delivery-policy.md (1)
85-103: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake evidence-output containment an explicit fail-closed seal invariant.
“External” output and a detached clone are not sufficient to prevent writes through candidate roots, sibling worktrees, shared Git metadata, or symlink/junction escapes. Require path resolution and rejection before commands run, with post-command verification covering all durable logs and evidence outputs.
Suggested policy clarification
+ Sealing fails closed unless every output, log, and evidence path resolves + outside the candidate root, all sibling worktrees, and shared Git metadata; + symlink/junction escapes are rejected before execution and rechecked afterward.This is required by the PR objective for fail-closed output containment across candidate roots, sibling worktrees, and shared Git metadata.
Also applies to: 197-204
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/delivery/greenfield/delivery-policy.md` around lines 85 - 103, Update the candidate-sealing requirements around “pnpm candidate:seal” to make evidence-output containment fail closed: resolve and validate the external output directory and all durable log/evidence paths before running commands, rejecting paths that resolve inside candidate roots, sibling worktrees, shared Git metadata, or symlink/junction escapes. Require post-command verification of every durable log and evidence output, and invalidate the seal on any containment violation or unexpected write.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/delivery/greenfield/delivery-policy.md`:
- Around line 85-103: Update the candidate-sealing requirements around “pnpm
candidate:seal” to make evidence-output containment fail closed: resolve and
validate the external output directory and all durable log/evidence paths before
running commands, rejecting paths that resolve inside candidate roots, sibling
worktrees, shared Git metadata, or symlink/junction escapes. Require
post-command verification of every durable log and evidence output, and
invalidate the seal on any containment violation or unexpected write.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a90db493-e64c-4526-bdf2-9e213125a12c
📒 Files selected for processing (7)
docs/delivery/greenfield/delivery-policy.mddocs/delivery/greenfield/reviewer/README.mddocs/delivery/greenfield/reviewer/review-checklist.mddocs/delivery/greenfield/story-contract.mddocs/delivery/greenfield/verification.mdscripts/seal-candidate.mjsscripts/seal-candidate.test.mjs
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/delivery/greenfield/reviewer/review-checklist.md
- docs/delivery/greenfield/reviewer/README.md
- docs/delivery/greenfield/verification.md
- docs/delivery/greenfield/story-contract.md
- scripts/seal-candidate.mjs
- scripts/seal-candidate.test.mjs
|
Re the CodeRabbit outside-diff output-containment suggestion: no further change is warranted. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de5953ba27
ℹ️ 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".
1 participant
Summary
Verification
pnpm candidate:seal:test— 12 focused boundary regressions passed1e994015cca98ea80e9e0f7607d02992b7894847, treedc4bf68ceb2e9da2f4c574a5aafd79e49f27bff7origin/main/4db373ec6067018e9724fcc51d29ee75044d212d; merge-base matched/private/tmp/jig-candidate-seal-1e99401/envelope.json: candidate-bound diff andpnpm checkexited 0; clone/original states clean;seal.valid: trueThe reviewer packet and checklist preserve read-only semantic authority; missing or stale evidence remains a finding rather than a reason to rerun verification.
Summary by CodeRabbit
check) workflow.