fix(drive-local): declare acceptance inputs as immutable (#284) - #294
Conversation
Session-Id: 01a08cf3-1384-7810-bdc7-6cfe60d0a4c1
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review swarm: maintainabilityMaintainability Review: PR #294 - Immutable Acceptance InputsPR: #294 SummaryThis PR addresses issue #284 by implementing an ownership contract for acceptance test inputs, preventing implementation code from mutating the checks that verify it. The change introduces a new module ( Maintainability Assessment1. UNCLEAR BOUNDARY: Acceptance vs. Verification SeparationFinding: The division of responsibility between
What would break: If someone adds a new kind of check, they could reasonably assume it belongs in Location: 2. IMPLICIT CONTRACT: Git Input Validation Happens TwiceFinding: Git input validation occurs in both The contract is:
Why it's implicit: Nothing documents that inputs with return { ...input, ...(input.ref === 'HEAD' ? { ref } : {}) };What would break: If someone adds a new ref type (e.g., Locations:
3. MISSING FAILURE HANDLING: Git Command Errors Become OpaqueFinding: Git errors are wrapped in generic violation messages that lose critical diagnostic context. In try {
assert.equal(git('cat-file', '-t', input.ref).toString().trim(), 'commit');
const mode = git('ls-tree', input.ref, '--', input.path).toString().split(' ')[0];
assert(mode === '100644' || mode === '100755');
return git('show', `${input.ref}:${input.path}`);
} catch (cause) {
throw new Error(violation(`pinned Git input unavailable: ${input.ref}:${input.path}`), { cause });
}The problem: This catch block conflates:
All produce: What would break: A stranger debugging why their acceptance input fails won't know if they:
The Location: 4. COMMENT THAT ASSERTS WHAT CODE DOES NOT DO: "Do not materialize a symlink blob"Finding: Line 221 claims: // Do not materialize a symlink blob as executable source.But the code that follows does NOT prevent materialization. It checks the Git mode and refuses symlinks, but if a symlink passes (somehow), the next line The comment asserts a safety property that depends on the mode check being exhaustive, but:
What a maintainer would miss: They might assume "do not materialize" means there's explicit handling to refuse materialization. In fact, it means "we assert the mode, so materialization won't happen." If the assertion is wrong, there's no second line of defense. Location: 5. UNCLEAR BOUNDARY: What "Writable Paths" IncludesFinding: The Lines 192-212 implement a visited-set traversal that:
Why it's unclear:
What would break: If someone needs to distinguish "paths that are lexically writable" from "paths that are writable through symlink aliases," they can't. The function mixes both into one array without tagging them. Location: 6. TEST THAT WOULD NOT FAIL IF BEHAVIOR BROKE: Hard Link TestFinding: The test at test('an external input with an in-scope hard link is refused', t => {
const f = fixture(t);
f.put('checks/check.cjs', 'process.exit(0);');
const path = join(f.root, 'checks/check.cjs');
linkSync(path, join(f.root, 'src/check.cjs'));
f.put('ops/BACKLOG.md', entry('Fix value', 'src/', [{ argv: ['node', path], inputs: [{ path }] }]));
commit(f);
pass(f.run('select'));
fail(f.run('verify'), /ACCEPTANCE_IMMUTABILITY_VIOLATION: external input has writable hard-link aliases/);
});The problem: This test creates a hard link from assert(stat.nlink === 1, violation(`external input has writable hard-link aliases: ${input.path}`));Only checks What would not be caught: If the code changed to allow hard links but still failed for some other reason (e.g., wrong path), this test would still pass because it only asserts the error message matches a regex. Location: 7. UNCLEAR BOUNDARY: When DoD Gets Appended to ErrorsFinding: The DoD (Definition of Done) is appended to errors in three places with different trigger conditions:
Why it's unclear:
What would break: If someone adds a new validation step (e.g., verifying backlog integrity), they might throw an error that doesn't include DoD context, creating inconsistent error quality. Locations:
8. IMPLICIT CONTRACT: Package Verification Order MattersFinding: The verification flow in
Why it's implicit:
What would break: If someone reorders these steps (e.g., runs checks before validation to "fail fast"), they would execute untrusted code before validating immutability, defeating the security property. Location: 9. MISSING FAILURE HANDLING: Extraction Directory Cleanup on Assertion FailureFinding: In for (const check of checks) {
const directory = mkdtempSync(join(tmpdir(), 'drive-acceptance-'));
try {
// ... extraction and execution ...
} finally { rmSync(directory, { recursive: true, force: true }); }
}The problem: If the process crashes or receives SIGKILL during check execution (line 303's Why this matters for maintainability: The code LOOKS like it cleans up (there's a finally block), but:
What would break: Over time on a long-running CI system, leaked temp directories would accumulate. A maintainer debugging disk usage wouldn't know these directories are from failed acceptance runs because they're in Location: 10. UNCLEAR BOUNDARY: When Ambient NODE_OPTIONS/NODE_PATH Are ExcludedFinding: Lines 300-302 delete const env = { ...process.env };
delete env.NODE_OPTIONS;
delete env.NODE_PATH;This happens for ALL checks, not just Node checks. The comment at line 258-259 says "Runtime options that could preload checkout code are deliberately excluded" but this applies to:
Why it's unclear: A shell script acceptance check doesn't care about
The implicit contract: The environment is "sanitized for all checks regardless of check type" but this isn't stated. It's implemented as "delete Node-specific vars for all checks." Location: Positive Observations
Maintainability Risk AssessmentHigh Risk:
Medium Risk:
Low Risk:
Could a Stranger Read This in Six Months and Change It Safely?Partial. A stranger could:
A stranger could NOT safely:
The code's core safety properties (immutability of acceptance inputs, scope enforcement) are sound, but the sequencing dependencies and implicit contracts create maintenance hazards. REVIEW_FAILED |
Review swarm: historyPR #294 — history reviewReviewed head: AssessmentNo blocking history finding. This change follows the acceptance-input decision left open by #244 rather than restoring the superseded working-tree snapshot design.
Review environment and limitsThe original The This is a static history review, not a new runtime acceptance certification, a claim that the full RFC gates are green, or authorization to merge. No application test suite or mutation test was run by this reviewer. The commands below capture the actual provenance and preservation checks used for this verdict. Captured commands and outputREVIEW_PASSED |
Review swarm: structureNo fresh transcript was produced for run |
Review swarm: FAILED
Cloud run: |
2 participants
An implementation agent could replace a checkout acceptance script with
process.exit(0)and obtainPACKAGE_VERIFIEDwhile the implementation remained broken. This adds a reconstructedverificationmanifest alongside #244's existing pinned baseline and argv: script inputs must come from full Git commit IDs or absolute paths outside implementation write scope. Git inputs are extracted afresh; undeclared scripts, missing pins, writable paths, symlink aliases, and hard-linked external inputs fail closed with an immutability diagnostic.Existing inline Node assertions remain supported. Verification emits
PACKAGE_VERIFIEDonly after its final scope check, and refusal diagnostics name the DoD. The launcher now uses the SDK'ssocketPathFor, matching the daemon socket relocation in #262 so the real journal tests can reach these gates.Validation:
npx --no-install tsc --noEmit; 68 local package/launcher tests; 51 SDK backlog/spec tests. The regression probe exits 1 withACCEPTANCE_IMMUTABILITY_VIOLATIONandIMPLEMENTATION=broken; isolated reporting refuses the outside edit and names the DoD. Literal commands and output: verification transcript.This enforces declared acceptance-input ownership. It does not sandbox trusted assertion programs or infer their dynamic dependencies; the existing same-user filesystem limitation remains documented.
Closes #284.
Note
High Risk
Changes verification gates that decide whether local drive work is complete; bugs could falsely pass broken implementations or block valid packages, though behavior is heavily tested.
Overview
Closes the #284 gap where an agent could swap in-scope acceptance scripts for no-ops and still get
PACKAGE_VERIFIED. Drive-local now builds a pinnedverification: { ref, checks: [{ argv, inputs }] }manifest from the backlog (alongside existingverificationCommands) and enforces it on initial scope, verify, and report.Script checks must declare
inputswith full Git commit IDs (or absolute paths outside write scope). Git bytes are extracted to a temp dir for execution; undeclared scripts, bad pins, writable/symlink/hard-link paths, and shell-cescapes fail withACCEPTANCE_IMMUTABILITY_VIOLATIONbefore any check runs. Inlinenode -eassertions stay as backlog-pinned code.PACKAGE_VERIFIEDis emitted only after acceptance plus a final scope pass; errors include the DoD.gate-snapshotnow pinslocal-work-acceptance.mjs. The local workflow launcher uses SDKsocketPathForfor daemon sockets (#262). Docs and broad tests cover the new contract and journal failure paths.Reviewed by Cursor Bugbot for commit 3e166fb. Bugbot is set up for automated code reviews on this repo. Configure here.