From cb0a73cad49425ae2774fe8c37c5af9975a0669a Mon Sep 17 00:00:00 2001 From: Yudhi Armyndharis Date: Wed, 12 Aug 2026 16:30:49 +0700 Subject: [PATCH] fix(policy): let the ref guard finish when history carries a policy it cannot read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `.aimhooman.json` this version cannot parse — a field a later version added, say — made the final ref guard refuse every update that would introduce it. Pull, merge, reset and `--no-verify` all aborted, and pulling the fix did not help either, because the revert's own scan threw on its parent's policy. Nothing printed pointed at a way out. History is not the receiving developer's to fix. Commit-target policy resolution now takes a tolerance flag: the guard asks for it, gets a result carrying the parse error instead of an exception, scans under the profile local config can reach, and reports which commit it could not read. A parent whose policy is unreadable still counts toward the strict floor, so an unparsable file cannot switch strict enforcement off. The worktree and the index keep the hard error, and so does anything the developer asked about directly — `check --commit` and `audit` should still say plainly that the file is unreadable. --- src/guard.mjs | 4 +++ src/policy-resolver.mjs | 29 ++++++++++++----- src/scan-target.mjs | 47 ++++++++++++++++++++++------ tests/policy-resolver.test.mjs | 57 ++++++++++++++++++++++++++++++++++ 4 files changed, 121 insertions(+), 16 deletions(-) diff --git a/src/guard.mjs b/src/guard.mjs index 5c79a5e..140ecdd 100644 --- a/src/guard.mjs +++ b/src/guard.mjs @@ -236,6 +236,10 @@ export function scanProposedCommits(repo, commits, { rejectNote, limits = {} }) policyMigrationContexts: reviewContexts, limits, messageScope: authoredLocally ? 'commit' : 'changes-only', + // This guard runs over history the developer is receiving, not + // writing. A policy it cannot parse must not wedge every pull, + // merge and reset with no way forward. + tolerateUnreadablePolicy: true, }); } catch (error) { diff --git a/src/policy-resolver.mjs b/src/policy-resolver.mjs index 0fa0dc4..1884b5c 100644 --- a/src/policy-resolver.mjs +++ b/src/policy-resolver.mjs @@ -38,7 +38,7 @@ export function resolvePolicy(repo, options = {}) { if (!repo?.root || !repo?.stateDir) { throw new TypeError('repo must be an open repository'); } - const { target = 'worktree', revision, profile, strictFloor } = options; + const { target = 'worktree', revision, profile, strictFloor, tolerant = false } = options; let resolved; if (target === 'worktree') { resolved = resolveWorktreePolicy(repo); @@ -48,11 +48,26 @@ export function resolvePolicy(repo, options = {}) { if (typeof revision !== 'string' || !revision) { throw new PolicyTargetError('commit target needs a revision'); } - resolved = resolveGitPolicy( - repo, - readCommitPath(repo, revision, POLICY_PATH), - 'commit-policy', - ); + const result = readCommitPath(repo, revision, POLICY_PATH); + try { + resolved = resolveGitPolicy(repo, result, 'commit-policy'); + } catch (error) { + // History is not the caller's to fix. A policy a future version + // wrote cannot be read here, and throwing turns every pull, merge + // and reset that carries it into a refusal with no way forward. The + // caller asks for tolerance and decides the fallback; the worktree, + // the index, and anything the developer asked about directly still + // get the loud error. + if (!tolerant || !(error instanceof ProjectPolicyError)) throw error; + return { + profile: null, + source: 'commit-policy', + target: result.target, + policy_object_id: result.oid, + policy_mode: result.mode, + unparseable: error.message, + }; + } } else { throw new PolicyTargetError('must be worktree, staged, or commit'); } @@ -141,7 +156,7 @@ function resolveGitPolicy(repo, result, source) { }; } -function localFallback(repo, target) { +export function localFallback(repo, target) { const config = loadConfig(repo.stateDir); return { profile: config.profile, diff --git a/src/scan-target.mjs b/src/scan-target.mjs index bea655a..f85f517 100644 --- a/src/scan-target.mjs +++ b/src/scan-target.mjs @@ -1,6 +1,6 @@ import { Engine, newEngineWithDiagnostics } from './scan.mjs'; import { loadOverrides } from './state.mjs'; -import { applyExplicitProfile, applyStrictFloor, resolvePolicy } from './policy-resolver.mjs'; +import { applyExplicitProfile, applyStrictFloor, localFallback, resolvePolicy } from './policy-resolver.mjs'; import { GitRevisionError, stagedEntries, @@ -155,19 +155,41 @@ export function resolveStagedPolicy(repo, explicitProfile) { function scanCommit(repo, options) { if (!options.revision) throw new TypeError('commit scan needs a revision'); const snapshot = commitSnapshot(repo, options.revision); - const rawPolicy = resolvePolicy(repo, { target: 'commit', revision: snapshot.commit }); + // The final ref guard scans history the developer did not write, so a policy + // this version cannot parse must not turn every pull, merge and reset into a + // refusal. It asks for tolerance; the profile then comes from local config + // and the reason is reported. Direct questions (check --commit, audit) still + // get the loud error. + const tolerant = Boolean(options.tolerateUnreadablePolicy); + const unreadablePolicies = []; + const resolvedPolicy = resolvePolicy(repo, { target: 'commit', revision: snapshot.commit, tolerant }); + if (resolvedPolicy.unparseable) unreadablePolicies.push(resolvedPolicy.unparseable); + const rawPolicy = resolvedPolicy.unparseable + ? { + ...localFallback(repo, resolvedPolicy.target), + policy_object_id: resolvedPolicy.policy_object_id, + policy_mode: resolvedPolicy.policy_mode, + } + : resolvedPolicy; // Bind policy-migration acks to the repository's current HEAD — the state an // ack is recorded against (policy-review --head --transition X) — // matching scanRange and resolveStagedPolicy. Probing with snapshot.commit // almost never matched a real ack, so `check --commit X` spuriously blocked // transitions that `check --range` honored. Exact-match security on every // ack field is unchanged; a null head simply fails closed (no ack honored). - const headBaseline = headPolicy(repo); + const headBaseline = headPolicy(repo, tolerant); const head = headBaseline?.target?.startsWith('commit:') ? headBaseline.target.slice('commit:'.length) : null; - const strictParentPolicies = snapshot.parents - .map((parent) => resolvePolicy(repo, { target: 'commit', revision: parent })) - .filter(isVersionedStrict); + const parentPolicies = snapshot.parents + .map((parent) => resolvePolicy(repo, { target: 'commit', revision: parent, tolerant })); + for (const parent of parentPolicies) { + if (parent.unparseable) unreadablePolicies.push(parent.unparseable); + } + // A parent whose policy cannot be read might have been strict. Counting it + // toward the floor keeps the guard conservative; ignoring it would let an + // unreadable file switch strict enforcement off. + const unreadableParent = parentPolicies.some((parent) => parent.unparseable); + const strictParentPolicies = parentPolicies.filter(isVersionedStrict); const policyMigrationContexts = options.policyMigrationContexts ?? (head ? [{ head, transition: snapshot.commit, @@ -181,7 +203,8 @@ function scanCommit(repo, options) { newMode: rawPolicy.policy_mode, }) )); - const floor = strictParentPolicies.length > 0 && !acknowledged && !isVersionedStrict(rawPolicy) + const floor = (strictParentPolicies.length > 0 || unreadableParent) + && !acknowledged && !isVersionedStrict(rawPolicy) ? 'parent-strict-floor' : null; const policy = effectivePolicy( @@ -205,6 +228,12 @@ function scanCommit(repo, options) { const accumulator = createAccumulator(options.limits); accumulator.addSkipped(loaded.skipped); const diagnostics = [...loaded.diagnostics]; + for (const message of unreadablePolicies) { + diagnostics.push({ + level: 'warning', + message: `${message}; scanned ${snapshot.commit} under the ${policy.profile} profile instead`, + }); + } if (snapshot.shallowBoundary) { const message = 'shallow repository: commit scan cannot prove parent policy; fetch full history (e.g. fetch-depth: 0)'; @@ -550,9 +579,9 @@ function effectivePolicy(raw, explicitProfile, floorSource, enforcedObjectIds = return applyExplicitProfile(floored, explicitProfile); } -function headPolicy(repo) { +function headPolicy(repo, tolerant = false) { try { - return resolvePolicy(repo, { target: 'commit', revision: 'HEAD' }); + return resolvePolicy(repo, { target: 'commit', revision: 'HEAD', tolerant }); } catch (error) { if (error instanceof GitRevisionError) return null; throw error; diff --git a/tests/policy-resolver.test.mjs b/tests/policy-resolver.test.mjs index f35b322..c1779cb 100644 --- a/tests/policy-resolver.test.mjs +++ b/tests/policy-resolver.test.mjs @@ -12,6 +12,7 @@ import { resolvePolicy, } from '../src/policy-resolver.mjs'; import { loadProjectPolicy, ProjectPolicyError, saveConfig } from '../src/state.mjs'; +import { scanGitTarget } from '../src/scan-target.mjs'; function freshRepo() { const dir = mkdtempSync(join(tmpdir(), 'aim-policy-target-')); @@ -199,3 +200,59 @@ test('worktree policy resolution rejects valid and dangling symlinks', { rmSync(dir, { recursive: true, force: true }); } }); + +// A policy this version cannot parse, anywhere in fetched history, used to make +// the final ref guard refuse every update that would introduce it — pull, reset +// and merge alike, with no message pointing at a way out. The guard needs a +// resolution it can carry on from; the loud error stays for the worktree, the +// index, and anything the developer asked about directly. +test('a commit policy this version cannot parse resolves tolerantly instead of throwing', () => { + const dir = freshRepo(); + try { + const repo = openRepo(dir); + writeFileSync(join(dir, '.aimhooman.json'), JSON.stringify({ schema_version: 1, profile: 'clean', exclude: ['vendor/**'] })); + execFileSync('git', ['add', '.aimhooman.json'], { cwd: dir }); + execFileSync('git', ['commit', '-q', '-m', 'unreadable policy'], { cwd: dir }); + + assert.throws( + () => resolvePolicy(repo, { target: 'commit', revision: 'HEAD' }), + ProjectPolicyError, + 'the direct question still gets the loud answer', + ); + + const tolerant = resolvePolicy(repo, { target: 'commit', revision: 'HEAD', tolerant: true }); + assert.equal(tolerant.profile, null, 'no profile can be read from it'); + assert.match(tolerant.unparseable, /unsupported field/, 'and the reason travels with the result'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +// The other half: the guard that receives history must be able to finish a scan +// of a commit whose policy it cannot read, under a profile it can reach, and say +// which commit it fell back on. +test('a commit scan tolerates an unreadable policy and reports the fallback', () => { + const dir = freshRepo(); + try { + const repo = openRepo(dir); + writeFileSync(join(dir, '.aimhooman.json'), JSON.stringify({ schema_version: 1, profile: 'clean', exclude: ['vendor/**'] })); + execFileSync('git', ['add', '.aimhooman.json'], { cwd: dir }); + execFileSync('git', ['commit', '-q', '-m', 'unreadable policy'], { cwd: dir }); + const head = execFileSync('git', ['rev-parse', 'HEAD'], { cwd: dir, encoding: 'utf8' }).trim(); + + assert.throws( + () => scanGitTarget(repo, { kind: 'commit', revision: head }), + ProjectPolicyError, + 'a direct scan still refuses loudly', + ); + + const scan = scanGitTarget(repo, { kind: 'commit', revision: head, tolerateUnreadablePolicy: true }); + assert.ok(scan.profile, 'the scan runs under a profile it could reach'); + assert.ok( + scan.diagnostics.some((d) => d.level === 'warning' && d.message.includes(head)), + 'and names the commit it could not read the policy of', + ); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +});