WS6 — Guidance Maturity: CLI authoring, staleness findings, import/export#33
Conversation
Add `clarion guidance create/edit/show/list/delete` plus the foundational
guidance-sheet storage write API that later WS6 tasks (T2/T4/T5) reuse.
Storage (clarion-storage):
- New `guidance.rs`: non-run-scoped `INSERT/UPSERT INTO entities` for
`kind = 'guidance'` sheets (operator-authored, no source-file anchor, outside
any analyze run — deliberately NOT the run-scoped writer-actor path).
Exposes upsert/get/list/delete + `guidance_sheet_matches_entity`. List sorts
`scope_rank ASC, authored_at ASC, id ASC` to mirror the read path.
- Lift `glob_match` from clarion-mcp into a shared `clarion_storage::glob` so
the read (`scope`/guidance match-rules) and write (CLI `--for-entity`)
surfaces share one matcher; clarion-mcp re-exports it (semantics unchanged).
CLI (clarion-cli):
- New `guidance.rs` handlers + `Guidance` subcommand. `--match` syntax is
`<type>:<value>` split on the first colon (subsystem/entity ids carry inner
colons), producing the `{"type":…}` match_rules shape the read path's
`rule_match` consumes. `edit` is read-modify-write: preserves authored_at /
provenance / pinned, changes only content. `authored_at` is minted from the
connection's own strftime clock (zero formatting drift vs created_at).
Scope: `promote` deferred to T2 (Filigree observation lifecycle); `--stale` /
`--expired` list flags deferred to T4. Cache invalidation left as
`// TODO(T-cache)` markers per the plan.
Tests: match-rule parsing (every type + malformed input) and scope-rank
ordering are the explicit TDD targets. 26 new tests (7 storage, 14 CLI unit,
5 CLI e2e). All workspace gates green per ADR-023 (fmt, clippy -D warnings,
build, nextest 994 passed).
REQ-GUIDANCE-01, REQ-GUIDANCE-03.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…1 review)
Two must-fix items from the T1 code-quality review.
FIX 1 — `--expires` silent-failure. The read path compares `expires` lexically
against a full `YYYY-MM-DDTHH:MM:SS.mmmZ` instant, so a raw date-only value
(`2026-06-03`) sorted below same-day instants and expired immediately, while
garbage (`tomorrow`) sorted above everything and never expired. `create` now
normalizes `--expires` through the connection's own
`strftime('%Y-%m-%dT%H:%M:%fZ', ?)` before the write — byte-format-identical to
`now` by construction (date-only → start-of-day UTC; offset forms → UTC `Z`;
unparseable input → rejected up front, mirroring `validate_scope_level`). Help
text documents the contract.
FIX 2 — rule-matcher drift guard. The CLI `--for-entity` matcher was a second
copy of the MCP `guidance_for` rule dispatch with nothing pinning them to agree.
Lifted the rule-match logic into `clarion-storage` as the single source of truth
(`MatchFacts` + `RuleVerdict` + `rule_match`); `clarion-mcp` now consumes it
(building facts from its already-loaded `EntityRow`, no extra lookup) and the
storage `bool` wrapper maps `Matched(_) → true`. Removes the duplication
structurally, mirroring the glob lift. `RuleVerdict` labels are preserved
verbatim (load-bearing for `guidance_for`'s `matched_by` + wardline-skip signal).
Polish: tightened the `guidance_sheet_matches_entity` doc to say it matches on
`match_rules` only (it ignores `guides`-edge composition that `guidance_for`
also honours); replaced `create`'s 7 positional args + `too_many_arguments`
allow with a `CreateArgs` struct; noted the non-atomic create-guard.
Tests (+4): normalize_expires_{produces_now_compatible_format,rejects_garbage,
future_is_not_lexically_expired} (unit) and create_normalizes_and_validates_
expires (e2e). The lift is exercised by the existing storage matcher test and
the MCP guidance_for read-path tests, both green.
ADR-023 gates green: fmt, clippy -D warnings, build, nextest (999 passed).
REQ-GUIDANCE-01, REQ-GUIDANCE-03.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cache) Authoring a guidance sheet now invalidates the cached summaries of the entities the sheet matches, so newly authored guidance reaches future prompts instead of staying inert until each entity's code changes (which is the only thing that currently rotates the summary_cache key). Per ADR-007's churn-eager invalidation: any guidance-sheet edit changes the composed guidance, so affected entities' cached summaries must be dropped. T1 left `// TODO(T-cache)` markers at the create/edit/delete sites; this wires them in. - clarion-storage: add `invalidate_summaries_for_sheet(conn, sheet, project_root) -> Result<usize>`. Drives off `SELECT DISTINCT entity_id FROM summary_cache` (the only entities that can be invalidated), reuses the lifted `guidance_sheet_matches_entity` matcher and the existing per-entity `delete_summary_cache_for_entity` DELETE — O(cached-entities), and dodges the SQLite 999-param ceiling a broad `path:` `IN (…)` would hit on a large corpus. kind='guidance' rows never carry cache rows, so they are excluded for free. - clarion-cli `guidance create|edit|delete`: invalidate after the write. `edit` invalidates the before/after match-set union (defensive; match_rules are immutable in edit today). `delete` snapshots the sheet before removal, then invalidates. Prints `Invalidated N cached summaries` (silent on 0). Scope is the eager wire-up only; general guidance_fingerprint computation stays out of scope. Tests: storage matched-drops/unmatched-survives + no-rule no-op; CLI e2e create-invalidates (path rule), non-matching-leaves-intact, and delete-invalidates (kind rule). Full suite: 1004 passed. REQ-GUIDANCE-03, ADR-007. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cosmetic only, no behavior change: - delete: drop redundant manual project_root canonicalization; pass the raw root and let invalidate_summaries_for_sheet canonicalize internally, matching create/edit. - invalidate_matched_summaries_union: correct the stated exact-union-count mechanism — pass 1's deletions remove those entities from pass 2's driving SELECT DISTINCT (not "idempotent DELETE contributes 0"). - create/edit: acknowledge the non-atomic sheet-write-then-invalidate posture (self-healing on re-run / next cache-key rotation). ADR-007, REQ-GUIDANCE-03. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ired + churn-stale (WS6 T4a)
Adds three analyze-time guidance-staleness signals (REQ-GUIDANCE-05, ADR-007;
WS6 plan T4a):
- CLA-FACT-GUIDANCE-ORPHAN (extended): emit_deletion_findings now also scans
guidance sheets' match_rules for {"type":"entity","id":X} entries whose X is in
the run's deleted set, in addition to the existing `guides`-edge scan. Both
sources merge into one de-duped sorted (sheet, target) set, so a sheet that
strands the same target via both paths emits exactly one finding. Confidence
stays deterministic (1.0); confidence_basis reworded to fit both sources.
- CLA-FACT-GUIDANCE-EXPIRED (new): one INFO finding (confidence 1.0) per guidance
sheet whose `expires` instant is lexically < now (same fixed-width
YYYY-MM-DDTHH:MM:SS.sssZ form iso8601_now emits). Absent/malformed expires skip.
- CLA-FACT-GUIDANCE-CHURN-STALE (new): one WARN finding (confidence 0.7,
heuristic) per sheet whose aggregate git_churn_count over matched entities meets
an asymmetric threshold — 20 for pinned sheets, 50 otherwise. Drives the churn
scan off `WHERE git_churn_count IS NOT NULL`, so it is honest-empty in
production (analyze does not populate churn in v1.0) and O(sheets × churned).
The true since-authored delta awaits the churn-history pipeline
(clarion-997c93ec4e); documented in a code comment.
EXPIRED + CHURN-STALE run in a new post-CommitRun pass
(emit_guidance_staleness_findings) placed OUTSIDE the SEI no_sei gate and
independent of any deletion, so --no-sei does not suppress them. Both new rule
ids are added to POST_RUN_FINDING_RULES so the Phase-8c emission relays them to
Filigree. Rule-catalogue rows added to detailed-design.md §5.
TDD: asymmetric pinned threshold (20–49 boundary), honest-empty churn,
expired past/future/none, match_rule orphan + guides/match_rule dedupe, and
EXPIRED-under---no-sei (SEI-independence guard). All four ADR-023 gates green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add two age/review-cadence filters to `clarion guidance list`: - `--expired` — only sheets whose `expires` is lexically in the past (`expires < now`), mirroring the MCP read path's expiry exclusion and the T4a `CLA-FACT-GUIDANCE-EXPIRED` finding byte-for-byte. No `expires` ⇒ not expired. - `--stale [--days N]` — only sheets not "touched" (the later of `reviewed_at` and `authored_at`) within N days (default 90). This is the review-cadence / age signal of system-design §7.741, NOT the churn-based `CLA-FACT-GUIDANCE-CHURN-STALE` finding (T4a) — code comments mark the distinction so the two are never conflated. Date logic lives in clarion-storage as two pure, unit-tested predicates (`guidance_sheet_is_expired`, `guidance_sheet_is_stale`); the CLI computes `now` and the `now − N days` cutoff once via the connection's own `strftime` (same fixed-width ISO-8601 shape stored timestamps use) and applies them in the existing list loop. Filters compose by intersection (AND) with each other and with `--for-entity`. Tests: storage unit tests for both predicates (incl. reviewed_at-wins, no-timestamp, strict-boundary cases); CLI e2e for --expired, --stale --days, and the expired∧stale intersection against a seeded DB. REQ-GUIDANCE-05; system-design.md §7 line 741; WS6 plan T4. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…3e74f0b) The guidance_for read tool compared a sheet's `expires` against the server clock with a raw lexical string compare. Production `serve` uses the default `unix:<seconds>` clock, which starts with 'u' (0x75); an ISO `expires` starts with '2' (0x32). Since '2' < 'u', `exp < now` was always true and EVERY sheet carrying any `expires` was wrongly treated as expired and hidden from consult agents. The suite masked this by injecting an ISO clock that production never uses. Replace the lexical compare with a parse-based one using the existing `parse_to_unix_seconds` helper (accepts both `unix:<secs>` and RFC3339). A sheet is expired only when both `expires` and `now` parse and the expiry precedes now; a missing/unparseable timestamp fails open (never hides a sheet). Widen `parse_to_unix_seconds` to `pub(crate)` so inspection.rs can reach it. Adds a regression test exercising the production `unix:` clock path: a far-future sheet survives, a far-past one is excluded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add `clarion guidance export --to <dir>` and `clarion guidance import
<dir>`, the committable-knowledge round-trip surface (REQ-GUIDANCE-06).
Storage (`clarion-storage::guidance`):
- New `PortableSheet { id, name, properties }` — the git-shareable form,
dropping per-DB `created_at`/`updated_at` (would inject spurious,
non-deterministic diffs) and the re-derivable `short_name`.
- `to_canonical_json`: deterministic, diff-friendly bytes. Sorted keys at
every depth (serde_json::Map is a BTreeMap in this build — no
preserve_order), pretty-printed (one field per line), trailing newline.
Arrays (match_rules) keep author order — that order is semantic.
- `file_name`: `:` → `__` so the colon-bearing entity id is a
filesystem-safe, injective, collision-free filename
(e.g. core__guidance__foo.bar.json). The id is authoritative inside the
file, so the encoding need not be reversible.
- `import_portable_sheet`: additive upsert (merge, never a destructive
mirror); re-derives short_name like the authoring path.
CLI (`clarion guidance`):
- export iterates sheets in stable id order, create_dir_all's the target,
writes one canonical file per sheet.
- import reads `*.json` only (a README/.gitignore in the dir is ignored,
not a dropped sheet), parses + upserts each, and fails LOUDLY naming any
malformed file (a silently-skipped sheet is data loss). ADR-007
churn-eager cache invalidation runs per imported sheet.
Tests: storage unit tests (determinism, sorted-keys, array-order,
round-trip, filename, malformed); CLI integration round-trip into a fresh
DB asserting every field, byte-determinism across two exports, idempotent
re-import, additive-not-mirror, malformed-file-names-the-file,
non-json-ignored. All five ADR-023 gates green (fmt, clippy, build,
nextest 1039 passed, doc).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Document what WS6 actually shipped for the guidance system and clearly mark the deferred pieces. Accuracy over optimism — operator-authored CLI guidance reaches consult agents via the `guidance_for` MCP read tool's query-time composition, NOT via auto-generated summaries (not yet wired: clarion-b8b296352e). - New docs/operator/guidance.md: the `clarion guidance` create/edit/show/list/delete/export/import workflow, `--match` / `--scope-level` / `--expires` semantics, scope-rank composition order, staleness (age `--stale` vs churn finding), and the non-pruning export caveat. Linked from docs/operator/README.md. (REQ-GUIDANCE-01, -03, -05, -06) - SKILL.md: replace the stale "guidance authoring is a future wave" blockquote — CLI authoring ships; agent-mediated propose/promote remains deferred (clarion-3d272da8a7); `guidance_for` is the read surface and composes authored sheets at query time. - cli.rs: one-line non-pruning caveat on `guidance export --to` help. Deferred and documented as such: propose/promote (clarion-3d272da8a7), wardline-derived guidance (clarion-115c93de0e, REQ-GUIDANCE-04), staleness-review UI (clarion-0d7e22c6cb, NG-13), and summary composition + real guidance_fingerprint (clarion-b8b296352e). CLA-FACT-GUIDANCE- CHURN-STALE is documented as inert until churn data is populated (clarion-997c93ec4e). detailed-design.md §5 already carries accurate EXPIRED + CHURN-STALE rows and the ORPHAN match_rules-extension note (T4a) — verified, not modified. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Minor bump for the WS6 guidance-maturity feature set. Updates the workspace version + crate path-deps, the Python plugin (pyproject.toml, plugin.toml, __init__.py, and the version-asserting tests), Cargo.lock, and the CLAUDE.md repo-state line. Cuts the CHANGELOG [Unreleased] section to [1.2.0] — 2026-06-03 (adding the WS6 guidance CLI / staleness findings / import-export / cache entries and the guidance_for expiry fix) and refreshes the compare links. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![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: 650cda23d8
ℹ️ 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".
…s-edge invalidation FINDING 1: upsert_guidance_sheet now rejects any id lacking the `core:guidance:` prefix before writing, so a hand-edited / malicious import naming a code entity (e.g. python:function:foo) can no longer overwrite that entity via ON CONFLICT(id) DO UPDATE. Defense-in-depth: the DO UPDATE is additionally scoped WHERE kind = 'guidance'. FINDING 2: the import CLI handler now snapshots the pre-import sheet and invalidates the UNION of old+new matches (mirroring the edit path), so an import that changes match_rules also drops the OLD-matched entities' stale cached summaries. FINDING 3: invalidate_summaries_for_sheet now also collects the sheet's `guides`-edge targets and invalidates their summary rows (union with the match-rule set, de-duped via the cached_ids-driven loop). The CLI delete path is reordered to invalidate BEFORE deleting the sheet row, since the guides edges are ON DELETE CASCADE and would otherwise vanish first. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f1f73d217
ℹ️ 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".
| if clarion_storage::guidance_sheet_matches_entity( | ||
| &conn, | ||
| sheet, | ||
| entity_id, | ||
| &canonical_root, | ||
| ) |
There was a problem hiding this comment.
When churn data is present, a sheet that applies only through an explicit guides edge is composed by guidance_for, but this scan only calls guidance_sheet_matches_entity, which evaluates match_rules only. In that context the aggregate stays 0 and CLA-FACT-GUIDANCE-CHURN-STALE is never emitted for high-churn guided entities; include edges(kind='guides') targets in this aggregation as the other guidance paths do.
Useful? React with 👍 / 👎.
| /// `core:guidance:` (nothing is written). Returns [`StorageError::Sqlite`] on | ||
| /// any `SQLite` failure (lock, constraint). | ||
| pub fn upsert_guidance_sheet(conn: &Connection, sheet: &GuidanceSheetInput<'_>) -> Result<()> { | ||
| if !sheet.id.starts_with(GUIDANCE_ID_PREFIX) { |
There was a problem hiding this comment.
This prefix-only guard still accepts imported IDs such as core:guidance:x/../../team, unlike the earlier non-guidance-ID issue; the fresh failure mode is that export later uses the id-derived file_name() under to_dir without stripping /, so a hand-edited sheet can make guidance export fail on missing intermediate directories or write through existing ones outside the intended export tree. Validate the full guidance slug/EntityId shape, or encode all path separators, before upserting imported sheets.
Useful? React with 👍 / 👎.
| id: &sheet.id, | ||
| name: &sheet.name, | ||
| short_name: sheet.short_name(), | ||
| properties: &sheet.properties, |
There was a problem hiding this comment.
When a shared JSON sheet is hand-edited or generated with a normal RFC3339 value such as 2026-06-03T09:00:00Z or an offset value, import stores properties verbatim, but guidance list --expired and the analyze-side expired finding compare expires lexically against the fixed .000Z clock shape. That makes expired imported sheets stay unflagged in common cases (for example ...09:00:00Z sorts after ...10:00:00.000Z at the Z vs . character), even though guidance_for parses and excludes them. Normalize or reject expires during import the same way create --expires does.
Useful? React with 👍 / 👎.
| upsert_guidance_sheet( | ||
| &conn, | ||
| &GuidanceSheetInput { | ||
| id: &id, | ||
| name: &slug, | ||
| short_name: &short_name, | ||
| properties: &properties, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
For clarion guidance create --match entity:<id>, this write only stores the JSON match rule and never inserts the reserved guides edge that the design says explicit entity targets should have. The sheet still composes by the fallback match-rule path, but graph consumers and guidance_for's guides_edge reason never see the explicit relationship for sheets created through the documented authoring path; create/import should keep the edges(kind='guides') rows in sync with entity match rules.
Useful? React with 👍 / 👎.
| conn.execute( | ||
| "INSERT INTO entities \ | ||
| (id, plugin_id, kind, name, short_name, properties, created_at, updated_at) \ | ||
| VALUES \ | ||
| (?1, 'core', 'guidance', ?2, ?3, ?4, \ | ||
| strftime('%Y-%m-%dT%H:%M:%fZ','now'), strftime('%Y-%m-%dT%H:%M:%fZ','now')) \ | ||
| ON CONFLICT(id) DO UPDATE SET \ | ||
| name = excluded.name, \ | ||
| short_name = excluded.short_name, \ | ||
| properties = excluded.properties, \ | ||
| updated_at = strftime('%Y-%m-%dT%H:%M:%fZ','now') \ | ||
| WHERE kind = 'guidance'", | ||
| params![sheet.id, sheet.name, sheet.short_name, properties], | ||
| )?; |
There was a problem hiding this comment.
When a sheet is created or imported after a project already has SEI populated, this insert only adds the entities row and never creates a sei_bindings entry. guidance_for now returns each composed sheet's own sei via sei_for_locator, so these first-class guidance entities always report null identity unless a test or manual SQL inserts the binding; mint/upsert the sheet's SEI alongside the entity write (and keep deletion in sync) rather than relying on the analyze SEI pass, which only processes scanned source descriptors.
Useful? React with 👍 / 👎.
The sprint-2 MCP-surface e2e asserted an exact 18-tool list that predates the WS5/WS5b catalogue tools (guidance_for … search_semantic), so it failed against the real list_tools() surface — a pre-existing failure on main, not introduced by WS6. Updated the expected list to the full 37-tool catalogue (order matches list_tools()) and made the PASS message count-agnostic so it won't re-stale. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f49943aa0
ℹ️ 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".
| /// A match rule (`<type>:<value>`); repeatable. | ||
| #[arg(long = "match", value_name = "RULE")] | ||
| r#match: Vec<String>, |
There was a problem hiding this comment.
Because --match is accepted as an unconstrained Vec, clarion guidance create --scope-level project --name global --content ... succeeds with zero rules; create then stores match_rules: [], while guidance_sheet_matches_entity returns false for empty arrays and guidance_for only composes sheets matched by a rule or a guides edge. In that scenario the operator has created an apparently project/global sheet that is listed and exportable but is never returned for any entity, so either require at least one match here or implement the design's implicit-global behavior for matchless project guidance.
Useful? React with 👍 / 👎.
| let dir = std::env::temp_dir(); | ||
| let file = dir.join(format!("clarion-guidance-{}.md", std::process::id())); | ||
| { | ||
| let mut f = std::fs::File::create(&file) | ||
| .with_context(|| format!("create temp edit file {}", file.display()))?; |
There was a problem hiding this comment.
On multi-user systems where $TMPDIR is shared, this predictable /tmp/clarion-guidance-<pid>.md path is opened with File::create, which follows existing symlinks and truncates the target before launching the editor. A local attacker can pre-create likely PID-named symlinks and cause clarion guidance edit to overwrite any file the operator can write; use an atomic temp-file API such as NamedTempFile/create_new in a private directory instead.
Useful? React with 👍 / 👎.
| let conn = Connection::open_with_flags( | ||
| &db_path, | ||
| OpenFlags::SQLITE_OPEN_READ_WRITE | OpenFlags::SQLITE_OPEN_URI, | ||
| ) | ||
| .with_context(|| format!("open database {}", db_path.display()))?; |
There was a problem hiding this comment.
This new write path opens .clarion/clarion.db directly and then lets create/edit/delete/import mutate it without calling the schema forward-compatibility guard that the writer path uses. If an older Clarion binary runs clarion guidance ... against a database already upgraded by a newer build (PRAGMA user_version greater than this build supports), these direct writes bypass the STO-02 protection and can corrupt an unknown future schema; verify user_version immediately after opening, or route through a storage opener that does.
Useful? React with 👍 / 👎.
…ync LLM provider refactoring - Fix macOS limits compilation in limits.rs using standard posix setrlimit. - Implement handle-locked safe_open TOCTOU mitigation in jail.rs and analyze.rs. - Thread-safe subprocess-spawning with child-death signal triggers in pyright_session.py. - Asynchronous LLM/Embedding provider traits and refactored MCP mocks to tokio channels/mutexes. - Clean ruff check/format and mypy strict checks for the Python plugin.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cb587bfa8
ℹ️ 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".
| } else { | ||
| uri | ||
| }; | ||
| path = Some(clean_uri.trim_start_matches('/').to_owned()); |
There was a problem hiding this comment.
When a SARIF producer emits a normal absolute URI such as file:///workspace/clarion/src/lib.rs, this strips the leading slash and posts workspace/clarion/src/lib.rs to Filigree instead of the repo-relative src/lib.rs (or the original absolute path). Since scan-results intake keys findings by path for updates/mark-unseen behavior, those findings are attached to a different file than Clarion's indexed paths and can be duplicated or left unseen; parse file:// URIs and relativize paths under project_root instead of trimming all leading slashes.
Useful? React with 👍 / 👎.
| if isinstance(expr, ast.Attribute): | ||
| return expr.attr == "TYPE_CHECKING" | ||
| if isinstance(expr, ast.BoolOp): | ||
| return any(_is_type_checking_guard(value) for value in expr.values) |
There was a problem hiding this comment.
For a guard like if TYPE_CHECKING or DEBUG: (or any other or expression containing TYPE_CHECKING), this marks imports in the body as type_only, even though that branch can execute at runtime when the other operand is true. The MCP circular-import and hotspot queries now filter type_only imports out, so these real runtime imports disappear from graph algorithms; only a bare TYPE_CHECKING guard, or boolean forms that are guaranteed false at runtime such as TYPE_CHECKING and ..., should be classified this way.
Useful? React with 👍 / 👎.
| .unwrap_or_else(|_| project_root.to_path_buf()); | ||
| let mut blocks = Vec::new(); | ||
| for sheet in list_guidance_sheets(conn)? { | ||
| if guidance_sheet_is_expired(&sheet, now) { |
There was a problem hiding this comment.
In production the MCP clock is unix:<seconds>, but this uses guidance_sheet_is_expired, which compares the sheet's ISO expires string lexically against that clock. Any expiry starting with 2 (including far-future values like 2999-...) sorts before u, so summary generation drops every expiry-bearing guidance sheet and computes an empty guidance fingerprint, unlike guidance_for which parses both formats. Convert the clock/expiry to comparable instants here before filtering.
Useful? React with 👍 / 👎.
- add Filigree observation transport plus MCP propose/promote guidance tools - add CLI guidance promote and promotion payload handling - derive Wardline guidance from real bundles and emit stale override findings - refresh docs, MCP tool counts, and e2e expected tool catalogue
… plugin - Introduced `tags` field in `RawEntity` and `EntityRecord` to store categorisation tags emitted by plugins. - Updated the `insert_entity` function to handle the persistence of tags in the database. - Enhanced the Python plugin to emit categorisation tags for entry points, HTTP routes, tests, and data models. - Implemented tests to verify that categorisation tags and docstrings are correctly emitted and persisted. - Updated documentation to reflect the new categorisation shortcuts and their behavior in the analysis process.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1961a647c1
ℹ️ 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".
| )); | ||
| } | ||
| }; | ||
| let authored_at = (self.clock)(); |
There was a problem hiding this comment.
When promote_guidance is invoked through MCP, the production clock is default_now_string() (unix:<seconds>), so this passes a unix: value into to_promoted_sheet and stores it as properties.authored_at. Guidance ranking and CLI age checks compare authored_at lexically against ISO timestamps, so MCP-promoted sheets sort after ISO-authored sheets and are never reported by guidance list --stale/age-based staleness. Use the same DB ISO clock as the CLI promotion path or normalize before writing.
Useful? React with 👍 / 👎.
| else { | ||
| return Ok(0); | ||
| }; | ||
| invalidate_summaries_for_sheet(&conn, &sheet, &project_root) |
There was a problem hiding this comment.
If promote_guidance overwrites an existing sheet id with different match_rules, this invalidates only the refetched post-upsert sheet, leaving cached summaries for entities that matched the old sheet stale. Fresh evidence beyond the earlier import-path finding is that this newly added MCP promotion path upserts directly and never snapshots the existing sheet before line 1265; mirror the CLI union invalidation so old∪new matches are cleared.
Useful? React with 👍 / 👎.
| if manifest.module_tiers.is_empty() { | ||
| for (key, entry) in &manifest.tier_entries { |
There was a problem hiding this comment.
When a real Wardline file contains both the tiers: guidance-map shape and any module_tiers, this branch skips every explicit tier entry and generates only module-tier sheets. The docs/tests support both shapes, so a repository that keeps path/content guidance under tiers while adding module_tiers silently loses those generated sheets and their summary guidance; generate the map entries independently of whether module-tier assignments are present.
Useful? React with 👍 / 👎.
| let client = FiligreeHttpClient::from_config(&mcp_config.integrations.filigree, |name| { | ||
| std::env::var(name).ok() | ||
| }) |
There was a problem hiding this comment.
In projects where Filigree publishes a live ephemeral port, serve and analyze emission first call resolve_filigree_url so they do not use a stale clarion.yaml base URL, but this new SARIF import path builds the client directly from config. In that setup clarion sarif import posts to the wrong port even though the integration is enabled; apply the same URL resolution before constructing the client.
Useful? React with 👍 / 👎.
| let client = FiligreeHttpClient::from_config_with_project_root( | ||
| &mcp_config.integrations.filigree, | ||
| |name| std::env::var(name).ok(), | ||
| Some(&canonical_root), | ||
| ) |
There was a problem hiding this comment.
clarion guidance promote also constructs its Filigree client from the static config URL, unlike the serve path that resolves .filigree/ephemeral.port before building the same client. When the dashboard is running on an ephemeral port, promotion cannot read the reviewed observation from /api/loom/observations and fails even though other Clarion/Filigree features work; resolve the live URL here before from_config_with_project_root.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32daf4b623
ℹ️ 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".
| if let Some(entity_id) = filters.for_entity | ||
| && !guidance_sheet_matches_entity(&conn, sheet, entity_id, &canonical_root) | ||
| .into_anyhow()? |
There was a problem hiding this comment.
When clarion guidance list --for-entity <id> is used on a sheet that applies through an explicit guides edge, this filter rejects it because guidance_sheet_matches_entity evaluates only match_rules. The MCP guidance_for path and the invalidation path both compose guides edges OR rule matches, so operators trying to inspect the guidance that applies to an entity will miss guides-only sheets even though they are active in summaries; include edges(kind='guides') targets in this filter as well.
Useful? React with 👍 / 👎.
| let driver_name = sarif | ||
| .get("runs") | ||
| .and_then(|r| r.as_array()) | ||
| .and_then(|r| r.first()) | ||
| .and_then(|r| r.get("tool")) | ||
| .and_then(|t| t.get("driver")) | ||
| .and_then(|d| d.get("name")) | ||
| .and_then(|n| n.as_str()) | ||
| .unwrap_or("unknown"); |
There was a problem hiding this comment.
When a SARIF file contains multiple runs from different tools and --scan-source is omitted, this derives one scan_source from only runs[0] and then posts findings from every run under that same source. That mis-scopes later runs for Filigree dedup/mark-unseen handling (for example Semgrep results in a CodeQL+Semgrep SARIF get tagged as codeql), so either process each run with its own driver-derived source or reject mixed-run imports unless the operator supplies an explicit source.
Useful? React with 👍 / 👎.
| "Wardline annotation group `{decorator}` is present in the fingerprint baseline. Preserve its Wardline review semantics when interpreting affected code." | ||
| )), | ||
| scope_level: Some("project".to_owned()), | ||
| match_rules: Some(vec![json!({"type": "wardline_group", "name": decorator})]), |
There was a problem hiding this comment.
For projects with wardline.fingerprint.json decorators, this creates a guidance sheet whose only rule is wardline_group; the current guidance read/summary paths treat that rule as unevaluable and only compose sheets that also match via a real rule or guides edge. These generated sheets are therefore listed/exported but never affect any entity's guidance or summaries, so add an evaluable target (or don't materialize them until wardline_group can be matched).
Useful? React with 👍 / 👎.
| if _is_module_level(parents) and node.name == "main": | ||
| tags.add("entry-point") | ||
| if _is_module_level(parents) and node.name in exported_names: | ||
| tags.add("exported-api") |
There was a problem hiding this comment.
Now that the Python plugin emits root tags such as entry-point and exported-api, find_dead_code no longer stays behind its empty-root guard, but its candidate set is still all_entity_ids, including core:file:*, guidance, findings, and other non-callable graph rows that are never reachable over call/import edges. In any indexed project with one tagged root, those non-code rows can be reported as dead code; restrict the candidate scan to code entities that can participate in the reachability graph.
Useful? React with 👍 / 👎.
| let Some((manifest_hash, manifest)) = read_manifest(project_root)? else { | ||
| return Ok(WardlineGuidanceStats::default()); |
There was a problem hiding this comment.
When wardline.yaml is removed after a previous analyze generated wardline_derived sheets, this returns before touching the existing guidance rows. Those pinned sheets still compose into summaries even though Wardline has been disabled/removed, and the staleness pass has no current_manifest_hash to flag them, so disabling Wardline leaves stale governance guidance active indefinitely; delete/expire or otherwise mark the generated sheets before returning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 941fea5a51
ℹ️ 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".
|
|
||
| let before = | ||
| get_guidance_sheet(&conn, &sheet.id).map_err(|err| anyhow::anyhow!("{err}"))?; | ||
| let mut write_sheet = true; |
There was a problem hiding this comment.
After the first analyze of any project with a Wardline manifest, before is Some but write_sheet still starts and remains true for unedited generated sheets, so every later analyze upserts the same sheet with a fresh authored_at and then runs the invalidation block below. That drops cached summaries for all matching entities even when the Wardline inputs did not change, causing unnecessary recomputation/cost and making the summary cache ineffective for Wardline-enabled repos.
Useful? React with 👍 / 👎.
| let conn = Connection::open_with_flags( | ||
| path, | ||
| OpenFlags::SQLITE_OPEN_READ_WRITE | OpenFlags::SQLITE_OPEN_URI, | ||
| )?; | ||
| conn.busy_timeout(std::time::Duration::from_secs(5))?; |
There was a problem hiding this comment.
This MCP promotion write path opens .clarion/clarion.db directly and mutates guidance rows without the schema::verify_user_version guard that the writer actor uses, so an older MCP server can write into a database whose PRAGMA user_version was advanced by a newer Clarion build. Fresh evidence beyond the CLI guidance-path comment is this separate MCP-only helper used by promote_guidance; add the same forward-compatibility check before upsert_guidance_sheet.
Useful? React with 👍 / 👎.
| OR json_valid(properties) = 0 \ | ||
| OR (COALESCE(json_extract(properties, '$.type_only'), 0) != 1 \ | ||
| AND COALESCE(json_extract(properties, '$.scope'), 'module') = 'module'))"; |
There was a problem hiding this comment.
When an imported or older edge row has malformed properties, this predicate is intended to include it (json_valid(properties) = 0), but SQLite still evaluates the later json_extract(...) terms in the OR expression and raises malformed JSON. In that case find_circular_imports and the coupling-hotspot import scan fail instead of treating malformed properties as runtime imports; wrap the extraction in a CASE WHEN json_valid(properties) branch or otherwise avoid calling json_extract on invalid JSON.
Useful? React with 👍 / 👎.
| tool: &str, | ||
| ) -> Result<serde_json::Value, FiligreeClientError> { | ||
| loop { | ||
| let frame = read_frame(reader, ContentLengthCeiling::DEFAULT).map_err(|err| { |
There was a problem hiding this comment.
If the spawned Filigree MCP command hangs, emits only notifications, or never answers the tools/call, this read_frame loop blocks forever; the configured Filigree HTTP timeout is not applied to this MCP fallback path. That can make propose_guidance/promote_guidance hang indefinitely on a broken sibling instead of returning the normal enrich-only error, so add a read/process timeout and kill the child on expiry.
Useful? React with 👍 / 👎.
Summary
Brings the guidance system from "schema baseline" to mature — the operator-driven authoring CLI, staleness signals as reviewable findings, deterministic team import/export, and the authoring→cache-invalidation tie-in. Implements REQ-GUIDANCE-03/-05/-06, ADR-007, ADR-024.
What ships:
clarion guidanceCLI (human authoring):create/edit($EDITOR) /show/list/delete/export/import. Match-rule syntax (path:/tag:/kind:/subsystem:/entity:), scope-levels,--expiresnormalization. Newclarion-storageguidance write API; rule-matcher lifted toclarion-storageas the single source of truth (MCP + CLI + analyze all consume it).clarion analyze):CLA-FACT-GUIDANCE-ORPHAN(extended tomatch_rules {entity:…}deleted-target orphans, was guides-edge only),CLA-FACT-GUIDANCE-EXPIRED(INFO),CLA-FACT-GUIDANCE-CHURN-STALE(WARN, conf 0.7, asymmetric 50/20-pinned). Surfaced vialist --stale(age/review-cadence) /--expired. CHURN-STALE is honestly inert untilgit_churn_countpopulation lands (clarion-997c93ec4e).guidance_forwas silently dropping every expiry-bearing sheet in production (unix:clock vs lexical ISO compare). Fixed to parse both formats; regression test under the production clock. (clarion-3153e74f0b)Intentionally deferred (tracked, not silently dropped):
wardline.yamlingest path yet (clarion-115c93de0e)guidance_fingerprint(clarion-b8b296352e) — until then authored guidance reaches agents viaguidance_for, not auto-summariesAlso filed: clarion-c6fb20b338 (pre-existing SKILL.md staleness re: WS5b tools).
Test Plan
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo build --workspace --binscargo nextest run --workspace --all-features— 1040 passed, 0 failedRUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps --all-featurescargo deny check🤖 Generated with Claude Code