feat(autonomy): deterministic prerequisite resolver (#2724) - #2809
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 2m 41s —— View job Security review (
|
| Severity | File | Summary |
|---|---|---|
| SUGGESTION | resolve-prerequisites.mjs#L220-L242 |
probeTracker joins the repo-controlled provider field (from .work-item-tracker.json) into a filesystem path without validating it, so a provider value containing ../ segments can make capsPath resolve outside repoRoot (path.join collapses ..). Impact is limited — it's only a boolean existence probe (statSync, no content is read/returned) and requires the invoker to already control repo content — but since this resolver composes signals for a fail-closed autonomy/guardrail verdict (ADR 0011), constraining provider to a safe identifier pattern before use is worth doing. Posted inline with a suggested fix. |
Reviewed and found sound (no issues):
- Declaration precedence logic (
resolveNeed/aggregateVerdict,resolve-prerequisites.mjs#L747-L833): abinding.jsondeclaration alone cannot force a"supported"verdict — a positive declaration only reaches"present"when an independent probe also ran and agreed; otherwise it degrades to"conditional". Negative/disabled declarations narrow but never widen. This matches the "declaration narrows; probe caps declarations" precedence the PR describes, and correctly stays fail-closed. - No command execution,
eval, template/SQL construction, or credential handling anywhere in the diff. - Test harness (
resolve-prerequisites.fixtures.test.mjs) usesspawnSyncwith an argument array (no shell), so no injection risk there. - Version bump / CHANGELOG / README changes are documentation-only.
![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: 26d9bce085
ℹ️ 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".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Last security-reviewed head: |
Closes #2724 Phase 4 of the routine-capability-detection plan (ADR 0011): resolve per-identity verdicts on a named surface from the generated emission, with precedence fixtures and wall-clock-free output. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
26d9bce to
42f99f7
Compare
|
Claude finished @cursor[bot]'s task in 6m 43s —— View job Code Review (
|
| Line | Finding |
|---|---|
resolve-prerequisites.mjs#L839 |
resolveIdentity never reads record.isolation_floor from the emission (verified present on every record, e.g. L2/L3) — verdicts are computed purely from needs, so an identity can resolve supported without the resolver ever checking whether the bound surface established the required isolation floor. |
resolve-prerequisites.mjs#L150 |
loadBinding collapses readJson's "unreadable" (malformed/corrupt binding.json) and "absent" (missing file) into the same no-binding fallback, silently dropping narrowing declarations on a parse failure rather than fail-loud/unresolvable. |
resolve-prerequisites.mjs#L673 |
probeMergePath's mergeCapable check accepts recorded.class === "temporal" on its own, but temporal denotes scheduling transport (per reference/trigger-dispatch.md), not merge capability — an advisory-only temporal surface satisfies merge_path regardless of merge_policy_capable. |
resolve-prerequisites.mjs#L453 |
probeSourceTree's fallback excludes only 3 doc filenames, so package.json/lockfiles/other manifests (already enumerated in this file's own DEPENDENCY_MANIFESTS) count as a source tree — verified tech-debt-sweep depends on source_tree as a need distinct from dependency_manifests/ecosystems, so this is a real false-positive path. |
All four were already raised by the Codex review and marked "Deferred follow-up.
Resolving to unblock merge." by @kyle-sexton — I'm not asking to block on them,
just confirming (with independent verification against the emission schema and
trigger-dispatch.md) that they remain live in the code and worth tracking to
resolution given this resolver composes fail-closed guardrail signals.
Reviewed and found sound, no new findings:
- Declaration-vs-probe precedence (
resolveNeed/aggregateVerdict) — matches
the documented "declaration narrows; probe caps declarations;
unresolvable ≠ absent" contract across all seven fixtures. - Fixture/test harness (self-policing manifest coverage, byte-identical
reproducibility check,spawnSyncwith an argument array) is correct and
matches the stated wall-clock-free guarantee. - Version bump / CHANGELOG / README changes are consistent and accurate.
) (#2813) Closes #2725 ## Summary Re-lands the autonomy setup-skill prerequisite-resolution slice (0.22.0) on main after #2809. Replaces #2804, which was auto-closed when its stack base was deleted on squash-merge. ## Related - Completes the autonomy stack after #2793 / #2809. - Supersedes closed #2804. --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
| const signals = []; | ||
| const findings = []; | ||
| for (const need of record.needs) { | ||
| const resolved = resolveNeed(need, record.identity, ctx); |
There was a problem hiding this comment.
Verdicts aren't gated on the emitted isolation_floor.
resolveIdentity only iterates record.needs; it never reads record.isolation_floor (present on every emission record, e.g. L2/L3 in generated/identity-prerequisites.json) or checks it against anything the bound surface actually established. So an identity can resolve supported purely from its needs list even when the scheduling surface never established the isolation floor the identity's class requires — e.g. the positive-verdict fixture resolves dependency-update-wave/mechanical as supported without the resolver ever consulting isolation posture.
If isolation-floor conformance is fully re-derived from the needs list per posture (so this is redundant rather than missing), a one-line comment saying so would help the next reader; otherwise this is a real fail-closed gap for a resolver whose stated purpose (ADR 0011) is composing guardrail-relevant signals.
This corroborates the existing (deferred) finding from the Codex review on this same line.
| const path = join(repoRoot, ".claude", "autonomy", "binding.json"); | ||
| const result = readJson(path); | ||
| if (!result.ok) { | ||
| return { |
There was a problem hiding this comment.
An unreadable binding.json is silently treated the same as an absent one.
readJson distinguishes reason: "absent" (ENOENT) from reason: "unreadable" (parse/read failure), but loadBinding collapses both into the same present: false, declarations: [] shape — the caller never sees result.reason. A binding.json that exists but is malformed (truncated write, merge-conflict markers, hand-edit typo) therefore silently loses every declaration it carries, including narrowing absent/disabled declarations, and probes can independently swing the verdict to supported.
For a resolver whose whole premise is fail-closed composition of trust-relevant signals (ADR 0011), a corrupt binding should fail loudly or resolve to unknown/unsupported — not silently degrade to "no binding at all," which is the more permissive of the two states.
This corroborates the existing (deferred) finding from the Codex review on this same line.
| // the surface is the repo-local half of the question. | ||
| const mergeCapable = | ||
| recorded.merge_policy_capable === true || | ||
| recorded.class === "temporal"; |
There was a problem hiding this comment.
merge_path is satisfied by class === "temporal" alone, independent of merge_policy_capable.
const mergeCapable =
recorded.merge_policy_capable === true ||
recorded.class === "temporal";Per reference/trigger-dispatch.md, temporal denotes the scheduling transport ("schedules and poll-fallback detectors for push-less surfaces"), not merge capability — an ordinary advisory-only/enqueue-only surface can just as validly be class: "temporal". Since merge_policy_capable already exists as the explicit signal for this need, the || recorded.class === "temporal" disjunct makes the check pass for any temporal surface regardless of that flag, which defeats the purpose of having it: a repo with a temporal surface bound for advisory-only dispatch would resolve merge_path as present for identities that require an actual merge-capable disposition.
This corroborates the existing (deferred) finding from the Codex review on this same line.
| (name) => | ||
| !name.startsWith(".") && | ||
| !["README.md", "LICENSE", "CHANGELOG.md"].includes(name), | ||
| ); |
There was a problem hiding this comment.
source_tree fallback treats build manifests as source code.
const codeish = entries.filter(
(name) =>
!name.startsWith(".") &&
!["README.md", "LICENSE", "CHANGELOG.md"].includes(name),
);This is meant to catch repos without one of the src/lib/app/pkg/cmd convention directories, but the exclusion list only names three docs — package.json, lockfiles (package-lock.json, pnpm-lock.yaml, ...), Cargo.toml, pom.xml, etc. all count as "codeish" here. A manifest-only repo root (package.json + CI + tracker config, no actual source) resolves source_tree as present, which is exactly the false-positive tech-debt-sweep guards against by requiring this need separately from dependency_manifests/ecosystems.
Reusing the DEPENDENCY_MANIFESTS list already defined at the top of this file to extend the exclusion set would close this gap without adding a new dependency.
This corroborates the existing (deferred) finding from the Codex review on this same line.
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…gger (#2822) ## Summary Reconciles the #2717 naming ruling with the already-shipped prerequisite-resolution chain. The ruling was posted 2026-08-16T01:54Z — about three hours **after** PR #2772 merged the contract document (2026-08-15T22:47Z) and after the downstream phases (#2783, #2809, #2813) had shipped a working vocabulary. Its premise ("the contract document may be authored against these") was stale when written, so the reconciliation decision re-derived the outcome: - **Verdict tokens ratified as shipped.** `supported` / `conditional` / `unsupported` / `unknown` clears every constraint that binds — no security-binding reading, no barred health word, no collision with the five named incumbents. The ruling's `met` family was a precision preference, not a defect cure; migrating ~60 sites across four merged PRs, including machine-emitted resolver tokens, buys no correctness. **No token changes.** - **The deferred marker gains its mandatory trigger: `deferred-class` → `deferred(<trigger>)`.** This half of the ruling names a genuine defect — a bare marker records that resolution is postponed while discarding the condition under which the deferral is revisited, the half that makes it auditable. The trigger is the `join:` row's own catalog Status trigger. 6 sites, 4 files: the contract document, the README bullet, ADR 0011 (a dated appended amendment — the original decision text is byte-identical, and the released CHANGELOG entries stay untouched as historical record), and a new 0.22.1 changelog entry with the version bump. A reader who finds the ruling comment and the shipped vocabulary disagreeing now finds the reconciliation on the record in ADR 0011's amendment. ## Test plan - `scripts/check-changelog-parity.sh --check-bump origin/main` exits 0; `plugin.json` bumped 0.22.0 → 0.22.1 with a matching `## [0.22.1]` entry - `markdownlint-cli2` and `typos` clean over all five changed files - `scripts/validate-plugins.sh` passes; `generate-catalog.mjs` / `generate-cheatsheet.mjs` report no drift - Repo-wide `git grep deferred-class` at the head commit hits only historical released changelog text and ADR 0011's original pre-amendment lines - Independently verified by a fresh-context auditor reading blobs at the pushed SHA with authoring rationale withheld: all nine checks PASS (its one finding — a changelog sentence misattributing the ratification to the ruling — is fixed in the head commit) ## Related Closes #2717 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Closes #2724
Summary
Replacement for #2796 (auto-closed when stack base was deleted after #2793 squash-merge). Lands the deterministic prerequisite resolver on main after #2793.
Closes #2724
Summary
Phase 4 of the routine-capability-detection plan (ADR 0011): deterministic prerequisite resolver that reads the Phase 3 emission and emits per-identity verdicts with provenance on a named scheduling surface.
Fix
skills/setup/scripts/resolve-prerequisites.mjs— repo-file + harness-context probes; composes binding declarations, ecosystems (resolved), tracker seam,.mcp.jsonpresence/enablement.scripts/fixtures/prerequisite-resolution/+ co-located test/manifest (bare-repo, fail-closed, declared-absent-narrows, probe-negative-caps, probe-could-not-run, posture-divergence, positive-verdict).0.21.0.Stacked on #2793 (
cursor/2723-prereq-emit-8f91).Verification
Related
Related