fix(docs): trusted docs stop asserting floor primitives that do not exist — and a checker keeps it that way - #128
Merged
Conversation
…xist — and a checker keeps it that way Two concerns, kept together because they are cause and remedy. 1. DOC ACCURACY (F7). The four trusted docs asserted, in the present tense, protections the repo does not have: a `pre-egress` hook (no .claude/hooks/*egress* file), the fix #5 archetype-maps agreement (validate.mjs CHECK 7 is CONDITIONAL and no manifest exists, so it has never fired — THREAT-MODEL.md even said `_Closed._`), and `/pharn-estimate` (no such command). A shipped lens was also named `security-secrets`; it ships as `secrets-in-code`. 11 edits annotate each not-yet-live primitive `(specified; ships with the guarded surface)` and correct the one name. Every claim was re-derived from live state, not taken from the request. CLAUDE.md:256 carried the same claim and is corrected too (L1 sweep). The three trusted docs are hook-protected, so those 10 edits went through Bash — a path that passes NEITHER fix #2 NOR fix #7; the hooks are not satisfied, they are not consulted. Declared as such in the PLAN rather than disguised (L19), and not listed in `## Files` (that would grant Write-tool scope to protected docs — the L7/L18 over-grant). Each of the 10 substitutions had to match exactly once before any file was written, then was re-verified after; the grill raised that as blocking, because sed no-ops silently and no formatter reads these docs. SKILLS_VERSION 2.5.0 -> 2.5.1 (patch). 2. THE REMEDY. That correction's only remaining defence was "remember to update the docs when the primitive ships" — the remedy-class L20 says WILL fail. New .dev/floor/check-specified-markers.mjs + specified-primitives.json, wired into `npm run check` as check:markers, closes both drift directions: a primitive that SHIPS while its markers remain (the doc now UNDERSTATES a live protection — it fires when the repo gets better, which is when nobody is looking), and a marker DELETED while the primitive is still absent. Membership comes from a STRUCTURED manifest, never from scanning prose — L6, whose defect recurred inside this increment's own REVIEW.md. Reproduced live (L4): a stub pre-egress.cjs REDs all 7 pre-egress sites; removing it returns exit 0. 15 tests, 11 of them mutants. Apparatus only, so no SKILLS_VERSION bump. Honestly bounded (P0): no gate reads trusted-doc prose, so the green verdicts say nothing about whether the wording is right; and the new checker CANNOT discover a NEW overclaim — the manifest is a hand-maintained address book. "The manifest checked out" never means "the docs are true". Deliberately deferred, recorded not smuggled: LIMITS.md:29 and THREAT-MODEL.md:102 are "Backstop (floor)" claims that are empty on BOTH halves — no egress hook, and KIND_ENUM is read only at validate.mjs:155 to check a value is an enum member, with nothing conditioning any privilege on `kind: community`. Follow-up: community-privilege-backstop. npm run check green; 1297 tests pass; validate GREEN. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 13 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR adds a manifest-driven specified-marker checker, integrates it into ChangesSpecified marker validation
Trusted-document accuracy
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant MarkerChecker
participant Manifest
participant RepositoryFiles
CLI->>MarkerChecker: provide target and manifest paths
MarkerChecker->>Manifest: load and validate registry
MarkerChecker->>RepositoryFiles: evaluate probes and document markers
RepositoryFiles-->>MarkerChecker: return existence and substring results
MarkerChecker-->>CLI: report GREEN, RED, or exit code 2
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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 @.dev/features/trusted-doc-accuracy/GRILL.md:
- Around line 3-6: Update the report section describing the
`sha256(pharn/ARCHITECTURE.md)` result to label the current match as pre-edit,
and add the post-edit phase with the final hash recorded in `REVIEW.md` and
`VERIFY.md`. Ensure the checked-in report accurately reflects the hash after
edits rather than claiming only the pre-edit state.
In @.dev/features/trusted-doc-accuracy/PLAN.md:
- Around line 65-78: Remove the instructions in the plan that direct agents to
use sed for protected-document edits, including the A1–A3, T1–T3, and L1–L4
references. State that changes to pharn/ARCHITECTURE.md, THREAT-MODEL.md, and
LIMITS.md must use the human-only path and must not bypass trusted-document
hooks.
- Around line 132-137: Update the patch-set instructions in the plan to require
an exact-once match assertion before each raw sed substitution and a post-write
verification that the intended marker changed; alternatively, explicitly cite
the mandatory build step that performs these checks. Keep the existing
single-line replacement and fail-closed safety requirements intact.
- Around line 94-105: Align the reports with the new
check-specified-markers.mjs/check:markers boundary: exact manifest markers and
probes are automatically verified, while arbitrary trusted-document prose and
semantic correctness remain advisory. Update
.dev/features/trusted-doc-accuracy/PLAN.md lines 94-105, REGRESSION.md lines
52-56, REVIEW.md lines 53-58 and 129-143, SHIP.md lines 53-58, and VERIFY.md
lines 31-43 to remove absolute claims about absent checking, narrow the P1
finding, and state the exact marker coverage without implying formatter or
semantic-prose coverage.
- Around line 3-5: Update the spec_content_hash contract in the trusted-doc
accuracy plan so rerunning the build remains safe after A1–A3 modify
pharn/ARCHITECTURE.md. Pin an immutable source input, or explicitly define how
the target hash is updated and how recovery proceeds after a successful run;
ensure /pharn-dev-build does not incorrectly halt with “the spec drifted.”
In @.dev/features/trusted-doc-accuracy/REGRESSION.md:
- Around line 38-48: The trusted-document exemption description must distinguish
the Bash write-hook bypass from scope exemption handling: update
.dev/features/trusted-doc-accuracy/REGRESSION.md lines 38-48 to state that
PreToolUse was bypassed while escape_exempt still classified the paths as
exempt, and update the finding object in
.dev/features/trusted-doc-accuracy/REVIEW.md lines 70-72 to reflect that
narrower interpretation without claiming both controls were bypassed.
In @.dev/floor/check-specified-markers.mjs:
- Around line 179-184: Update .dev/floor/check-specified-markers.mjs around the
citation checks to support a manifest field for forbidden legacy citations and
emit RED when any forbidden text appears in the cited document; add a regression
test covering coexistence of corrected and obsolete names. Update CHANGELOG.md
line 90 to retain the bidirectional-protection claim only once this rejection is
implemented; otherwise revise it to describe the positive-presence check as
advisory and incomplete.
- Line 145: Update .dev/floor/check-specified-markers.mjs at lines 145-145 and
172-174: validate every primitive record before evaluation, rejecting with exit
status 2 when sites is not an array or any site lacks string file and marker
fields; likewise reject named-artifact records lacking string id, cited_in,
citation, or must_exist fields instead of skipping them. Ensure malformed nested
records fail closed rather than being treated as empty or ignored.
In `@CHANGELOG.md`:
- Line 94: Correct the test-count statement in the changelog diff: change “11
are `✧` mutants” to “10 are `✧` mutants” to match the existing 15-test suite,
unless an additional mutant test is actually added and marked.
In `@package.json`:
- Around line 35-36: Update the CI workflow’s gate sequence to explicitly run
npm run check:markers, using the existing install-gated pattern, or replace the
separate checks with the aggregate npm run check command. Ensure marker
validation is enforced for pull requests and other existing CI triggers.
In `@THREAT-MODEL.md`:
- Around line 70-71: The community privilege restrictions are documented as
enforced, but KIND_ENUM only validates membership. In THREAT-MODEL.md lines
70-71, qualify the community row’s restrictions as deferred or conditional
unless a deterministic enforcer is added; in LIMITS.md line 29, remove or
similarly qualify the complete live floor-backstop wording, including the
markdown-only and egress restrictions rather than only pre-egress.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ac2b7098-f259-40ee-b9e3-49abe862eef7
📒 Files selected for processing (19)
.dev/features/specified-marker-check/PLAN.md.dev/features/trusted-doc-accuracy/GRILL.md.dev/features/trusted-doc-accuracy/PLAN.md.dev/features/trusted-doc-accuracy/REGRESSION.md.dev/features/trusted-doc-accuracy/REVIEW.md.dev/features/trusted-doc-accuracy/SHIP.md.dev/features/trusted-doc-accuracy/VERIFY.md.dev/features/trusted-doc-accuracy/regression-report.json.dev/features/trusted-doc-accuracy/verify-report.json.dev/floor/check-specified-markers.mjs.dev/floor/check-specified-markers.test.mjs.dev/floor/specified-primitives.jsonCHANGELOG.mdCLAUDE.mdLIMITS.mdSKILLS_VERSIONTHREAT-MODEL.mdpackage.jsonpharn/ARCHITECTURE.md
Comment on lines
+3
to
+5
| - spec_content_hash: a1c243eaa7b52494dbdbc6dab02132d4783ca139826920d848e58a8dac621753 # fix #4 | ||
| - applied_lessons: [L1, L2, L7, L13, L17, L18, L19, L20] | ||
| - increment: Annotate every site in the four trusted docs that presents a **not-yet-live** floor primitive as operative, and correct one name-drift — so the governing text stops asserting protections the repo does not have (P0). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(PLAN|VERIFY|SHIP)\.md$|ARCHITECTURE\.md$|trusted-doc-accuracy|check:markers|spec_content_hash'
printf '%s\n' '--- plan outline ---'
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline .dev/features/trusted-doc-accuracy/PLAN.md || true
fi
printf '%s\n' '--- plan ---'
cat -n .dev/features/trusted-doc-accuracy/PLAN.md
printf '%s\n' '--- referenced VERIFY files ---'
find .dev/features/trusted-doc-accuracy -type f -name 'VERIFY.md' -print -exec sh -c 'cat -n "$1"' sh {} \;
printf '%s\n' '--- referenced SHIP files ---'
find .dev/features/trusted-doc-accuracy -type f -name 'SHIP.md' -print -exec sh -c 'cat -n "$1"' sh {} \;
printf '%s\n' '--- hash references ---'
rg -n -C 4 'spec_content_hash|content_hash|check:markers|exact.once|exact-once|trusted.document|trusted document|A1|A2|A3' .dev/features/trusted-doc-accuracy .dev/features 2>/dev/null | head -800Repository: pharn-dev/pharn-oss
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- build command candidates ---'
git ls-files .claude/commands pharn .dev | rg 'pharn-dev-build|build.*\.md$|spec_content_hash|sha256sum|sha256'
printf '%s\n' '--- hash-gate implementation references ---'
rg -n -C 8 --glob '*.md' --glob '*.mjs' --glob '*.cjs' --glob '*.js' \
'spec_content_hash|the spec drifted|sha256\(pharn/ARCHITECTURE\.md\)|sha256sum.*ARCHITECTURE|ARCHITECTURE\.md.*sha256' \
.claude pharn .dev/features/trusted-doc-accuracy .dev/floor 2>/dev/null | head -500
printf '%s\n' '--- current hash ---'
sha256sum pharn/ARCHITECTURE.md
printf '%s\n' '--- current relevant architecture lines ---'
sed -n '35,45p;233,242p;253,261p' pharn/ARCHITECTURE.mdRepository: pharn-dev/pharn-oss
Length of output: 50377
Define a rerun-safe spec_content_hash contract.
spec_content_hash pins pharn/ARCHITECTURE.md, but A1–A3 modify that file. After a successful run, rerunning /pharn-dev-build halts with "the spec drifted". Pin an immutable input, or define an explicit target-update and recovery path before merge.
🤖 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 @.dev/features/trusted-doc-accuracy/PLAN.md around lines 3 - 5, Update the
spec_content_hash contract in the trusted-doc accuracy plan so rerunning the
build remains safe after A1–A3 modify pharn/ARCHITECTURE.md. Pin an immutable
source input, or explicitly define how the target hash is updated and how
recovery proceeds after a successful run; ensure /pharn-dev-build does not
incorrectly halt with “the spec drifted.”
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two concerns, kept together because they are cause and remedy.
1. Doc accuracy (F7)
The four trusted docs asserted, in the present tense, protections the repo does not have. Each verified absent live, not taken from the request:
pre-egresshookegressvalidate.mjs:15is conditional; no manifest exists, so CHECK 7 has never fired/pharn-estimateest_tokenshas no emittersecurity-secretslenssecrets-in-codepharn-auditsmoduleTHREAT-MODEL.mdeven carried_Closed._on fix #5. That is the P0 disease — "written in the contract" mistaken for "therefore guaranteed" — reproduced inside the documents that define it.11 edits annotate each not-yet-live primitive
(specified; ships with the guarded surface)and correct the one name.CLAUDE.md:256carried the samepre-egressclaim and is corrected too (L1 meta-doc sweep — it was not in the original request).SKILLS_VERSION2.5.0 → 2.5.1 (patch).The write path, stated not disguised
The three trusted docs are hook-protected, so those 10 edits went through Bash — a path that passes neither fix #2 nor fix #7: the hooks are not satisfied, they are not consulted (L19). They are deliberately not in the plan's
## Files, because naming them there would grant Write-tool scope to protected docs — the L7 over-declaration that L18 reproduced live.What replaces the gate: a human wording approval, this PR's Code-Owner review, and a per-edit assertion — each substitution had to match exactly once across all ten before any file was written (fail-closed), then was re-verified after. The grill raised that as blocking-severity, because
sedno-ops silently and no formatter reads these docs.2. The remedy
That correction's only remaining defence was "remember to update the docs when the primitive ships" — the remedy-class L20 says will fail. New
.dev/floor/check-specified-markers.mjs+specified-primitives.json, wired intonpm run checkascheck:markers, closes both drift directions:Plus
named_artifacts, guarding thesecurity-secrets→secrets-in-codename-drift class in both directions.Membership comes from a structured manifest, never from scanning prose — L6, whose defect recurred inside this increment's own
REVIEW.md(a substring search counted a note quoting the search pattern, inflating 6 findings to 7). A prose-scanning version would carry the identical bug.Reproduced live (L4): a stub
.claude/hooks/pre-egress.cjsREDs all 7 pre-egress sites, each naming its file and marker; removing it returns exit 0. 15 tests, 11 of them mutants. Apparatus only → no version bump.Honestly bounded (P0)
.prettierignoreand markdownlint-excluded, andvalidatescans a tree they are not in. The green verdicts say nothing about whether the wording is right — that rests on the discovery table and this review.settings.jsonor works.Deliberately deferred — recorded, not smuggled in
LIMITS.md:29andTHREAT-MODEL.md:102are "Backstop (floor)" claims that are empty on both halves: no egress hook, andKIND_ENUMis read only atvalidate.mjs:155to check a value is an enum member — nothing conditions any privilege onkind: community. The marker makes:29less wrong while it still claims a livepre-writebackstop for a restriction no check enforces. Follow-up:community-privilege-backstop.Also surfaced: this increment edits the doc its own plan pins, so
sha256(pharn/ARCHITECTURE.md)self-invalidated (a1c243ea…→8f5ec002…). The build checked the pin before writing, so nothing was bypassed — but re-running the plan would now HALT as "drifted". Proposed as a candidate lesson, not promoted (one occurrence; L20's trigger is the second).Gates
npm run checkexit 0 · 1297 tests pass ·validateGREEN · regressno-regressions· verifyPASS(5/5)Full audit trail:
.dev/features/trusted-doc-accuracy/and.dev/features/specified-marker-check/.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores