Implement scoped graph freshness surfaces#478
Conversation
Expose overall and selected-context freshness across pack, prompt, handoff, doctor/status, and MCP surfaces; add the scoped strict flag; refresh cached context_pack freshness; and document the new contract. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR implements graph freshness detection and enforcement across Madar's context pack, prompt, handoff, and diagnostic surfaces. It introduces freshness status types, scoped analysis that separates global graph staleness from selected-context staleness, optional enforcement flags, and consistent reporting in human-readable and machine-readable output to help users trust whether the graph is fresh enough for their task. ChangesGraph Freshness Detection and Enforcement
Sequence Diagram(s)sequenceDiagram
participant Client
participant CLI_MCP as CLI/MCP
participant Analyzer as analyzeGraphContextFreshness
participant Retrieval as retrieveContext
participant Enforcer as requireFresh*
participant Governance as BuildGovernance/Render
Client->>CLI_MCP: call context_pack/context_prompt (require_fresh_*)
CLI_MCP->>Analyzer: compute initial freshness
Analyzer-->>CLI_MCP: GraphContextFreshness
CLI_MCP->>Enforcer: enforce requireFreshGraph (optional)
Enforcer-->>CLI_MCP: pass or throw
CLI_MCP->>Retrieval: retrieve selected context
Retrieval->>Analyzer: recompute freshness with selected files
Analyzer-->>Enforcer: selected-context status
Enforcer-->>CLI_MCP: pass or throw
CLI_MCP->>Governance: build governance receipt with graph_freshness
Governance-->>Client: response with graph_freshness (JSON + human text)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/infrastructure/handoff-command.ts (1)
174-181: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUpdate the
runContextPackCommandtype signature to include the new optional fields.The
runContextPackCommandsignature inHandoffCommandDependenciesshould includerequireFreshGraph?: booleanandrequireFreshContext?: booleanto match thePackCliOptionscontract. Lines 209-210 now conditionally pass these fields through topackOptions, but the dependency interface type doesn't declare them, creating a gap for test mocks and custom dependency implementations.📐 Proposed fix to align the type contract
export interface HandoffCommandDependencies extends Partial<ContextPackCommandDependencies> { loadGraph?: (graphPath: string) => KnowledgeGraph runContextPackCommand?: (options: { prompt: string budget: number task: HandoffCliOptions['task'] graphPath: string format: 'json' verbose: true + requireFreshGraph?: boolean + requireFreshContext?: boolean }) => Promise<string> }🤖 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 `@src/infrastructure/handoff-command.ts` around lines 174 - 181, The runContextPackCommand signature on HandoffCommandDependencies is missing the new optional flags from PackCliOptions; update the type for runContextPackCommand (the function defined on HandoffCommandDependencies) to accept requireFreshGraph?: boolean and requireFreshContext?: boolean in its options object so mocks and implementations can pass them through when building packOptions (the place that conditionally forwards these fields to packOptions). Ensure the optional boolean fields are declared alongside prompt, budget, task, graphPath, format, and verbose in the runContextPackCommand options type.
🧹 Nitpick comments (4)
src/runtime/freshness.ts (1)
351-352: 💤 Low value
generated_msuses graph file mtime, not actual generation timestamp.The
generated_msandgenerated_atfields are derived fromgraphFreshness.graphModifiedMs(the graph file's mtime), which may differ from the actual generation timestamp if the graph file was touched or copied after generation. If this is intentional (using mtime as a proxy), consider adding a brief comment. If the graph JSON contains a generation timestamp field, that would be more accurate.🤖 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 `@src/runtime/freshness.ts` around lines 351 - 352, The generated_ms and generated_at fields are currently populated from graphFreshness.graphModifiedMs (file mtime) which can differ from the actual generation timestamp; update the code that sets generated_ms/generated_at to use the graph's embedded generation timestamp if present (e.g., a field like graph.generation_ts or graph.generatedAt in the parsed graph JSON), falling back to graphFreshness.graphModifiedMs only when that field is absent, and if you intentionally keep mtime as the source add a brief comment next to the generated_ms/generated_at assignment explaining that mtime is used as a proxy.tests/unit/freshness-surfaces.test.ts (1)
33-36: ⚡ Quick win
writeTexthelper unused intests/unit/freshness-surfaces.test.ts(33-36). The fixtures write files viawriteFileSyncdirectly, andwriteTexthas no call sites in this file; remove it if it’s unused elsewhere.🤖 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 `@tests/unit/freshness-surfaces.test.ts` around lines 33 - 36, The helper function writeText is declared but never used in this test; either remove the writeText function declaration or refactor the test to use writeText instead of calling writeFileSync directly. Locate the writeText function and any direct writeFileSync usages in the freshness-surfaces.test module, then either delete the writeText symbol or replace the raw writeFileSync calls with writeText(path, content) to consolidate file-writing logic and avoid an unused helper.src/infrastructure/context-pack-command.ts (1)
537-568: 💤 Low valueRedundant field deletions in cli_pack block.
The fields
graph_path,graph_version,generated_ms,generated_at,madar_version, andrecommendationare already deleted unconditionally at lines 441-472 before thiscli_pack-specific block runs. These deletions will never find the fields present.Consider consolidating this logic or removing the duplicate checks.
🤖 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 `@src/infrastructure/context-pack-command.ts` around lines 537 - 568, The cli_pack-specific deletion block redundantly re-checks and deletes fields on graphFreshness that were already unconditionally removed earlier (graph_path, graph_version, generated_ms, generated_at, madar_version, recommendation), causing dead checks; inside the same function where graphFreshness is processed (the cli_pack block that mutates trimmedFields), remove the duplicate conditional deletes for those already-handled keys or merge the two blocks into a single responsibility so each field is deleted exactly once—update the trimmedFields pushes accordingly and keep any unique cli_pack-only deletions (leave checks for fields not removed earlier).src/runtime/stdio/tools.ts (1)
1464-1482: 💤 Low valueConsider using
graphContextFreshnessfor impact task governance.The impact task governance uses
initialGraphContextFreshness(Line 1471) but therequire_fresh_contextenforcement at Lines 1431-1441 usesgraphContextFreshnesswhich includes selected source files from retrieval. This creates a slight inconsistency where the enforcement is based on retrieval's selected context, but governance reports freshness without selected context info.If intentional (impact has different "selected context" semantics), this is fine. Otherwise, consider using
graphContextFreshnessfor consistency.🤖 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 `@src/runtime/stdio/tools.ts` around lines 1464 - 1482, The governance payload for the impact task is using initialGraphContextFreshness but enforcement earlier uses graphContextFreshness; update the call that builds the impact task governance (the withContextPackGovernance invocation inside the helpers.ok(...) return) to pass graphContextFreshness instead of initialGraphContextFreshness so reported freshness matches the enforcement context (replace initialGraphContextFreshness with graphContextFreshness in the governance metadata for the impact task).
🤖 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 `@src/infrastructure/context-prompt-command.ts`:
- Around line 85-87: The requireFreshGraph check currently runs after retrieval
completes; move the early-fail check so it runs immediately after computing
initialGraphFreshness (i.e., right after the initialGraphFreshness is set) to
mirror runContextPackCommand and avoid doing retrieval work when
options.requireFreshGraph is true; specifically relocate the
options.requireFreshGraph / requireFreshGraph(initialGraphFreshness) invocation
to just after initialGraphFreshness is calculated and before any retrieval or
network calls in the same function.
In `@src/runtime/freshness.ts`:
- Around line 179-184: The loop over (fallbackNodes.length > 0 ? fallbackNodes :
result.matched_nodes) directly calls node.source_file.trim(), which can throw if
source_file is null/undefined; change the access to mirror earlier usage by
normalizing with optional chaining and a fallback string (e.g., use
node?.source_file?.trim() ?? '') before checking length and adding to
sourceFiles so fallbackNodes/result.matched_nodes entries are handled safely;
update the loop body that references sourceFile, keeping the same variable names
(sourceFile, sourceFiles, node) for clarity.
In `@tests/unit/context-prompt-command.test.ts`:
- Around line 6-15: The status regex in expectGraphFreshnessContract currently
omits the valid "partially_stale" enum value causing legitimate outputs to fail;
update the matcher inside expectGraphFreshnessContract so the status regex
includes "partially_stale" (e.g., change the pattern to
/^(fresh|partially_stale|possibly_stale|stale|missing)$/) so the contract
accepts the full set of allowed statuses.
---
Outside diff comments:
In `@src/infrastructure/handoff-command.ts`:
- Around line 174-181: The runContextPackCommand signature on
HandoffCommandDependencies is missing the new optional flags from
PackCliOptions; update the type for runContextPackCommand (the function defined
on HandoffCommandDependencies) to accept requireFreshGraph?: boolean and
requireFreshContext?: boolean in its options object so mocks and implementations
can pass them through when building packOptions (the place that conditionally
forwards these fields to packOptions). Ensure the optional boolean fields are
declared alongside prompt, budget, task, graphPath, format, and verbose in the
runContextPackCommand options type.
---
Nitpick comments:
In `@src/infrastructure/context-pack-command.ts`:
- Around line 537-568: The cli_pack-specific deletion block redundantly
re-checks and deletes fields on graphFreshness that were already unconditionally
removed earlier (graph_path, graph_version, generated_ms, generated_at,
madar_version, recommendation), causing dead checks; inside the same function
where graphFreshness is processed (the cli_pack block that mutates
trimmedFields), remove the duplicate conditional deletes for those
already-handled keys or merge the two blocks into a single responsibility so
each field is deleted exactly once—update the trimmedFields pushes accordingly
and keep any unique cli_pack-only deletions (leave checks for fields not removed
earlier).
In `@src/runtime/freshness.ts`:
- Around line 351-352: The generated_ms and generated_at fields are currently
populated from graphFreshness.graphModifiedMs (file mtime) which can differ from
the actual generation timestamp; update the code that sets
generated_ms/generated_at to use the graph's embedded generation timestamp if
present (e.g., a field like graph.generation_ts or graph.generatedAt in the
parsed graph JSON), falling back to graphFreshness.graphModifiedMs only when
that field is absent, and if you intentionally keep mtime as the source add a
brief comment next to the generated_ms/generated_at assignment explaining that
mtime is used as a proxy.
In `@src/runtime/stdio/tools.ts`:
- Around line 1464-1482: The governance payload for the impact task is using
initialGraphContextFreshness but enforcement earlier uses graphContextFreshness;
update the call that builds the impact task governance (the
withContextPackGovernance invocation inside the helpers.ok(...) return) to pass
graphContextFreshness instead of initialGraphContextFreshness so reported
freshness matches the enforcement context (replace initialGraphContextFreshness
with graphContextFreshness in the governance metadata for the impact task).
In `@tests/unit/freshness-surfaces.test.ts`:
- Around line 33-36: The helper function writeText is declared but never used in
this test; either remove the writeText function declaration or refactor the test
to use writeText instead of calling writeFileSync directly. Locate the writeText
function and any direct writeFileSync usages in the freshness-surfaces.test
module, then either delete the writeText symbol or replace the raw writeFileSync
calls with writeText(path, content) to consolidate file-writing logic and avoid
an unused helper.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 448454ff-672e-4169-b83c-2bef346a1afb
📒 Files selected for processing (23)
README.mddocs/concepts/context-packs.mddocs/reference/cli-and-mcp.mdsrc/cli/main.tssrc/cli/parser.tssrc/contracts/context-pack.tssrc/infrastructure/context-pack-command.tssrc/infrastructure/context-prompt-command.tssrc/infrastructure/doctor.tssrc/infrastructure/handoff-command.tssrc/runtime/context-pack-governance.tssrc/runtime/freshness.tssrc/runtime/stdio/definitions.tssrc/runtime/stdio/tools.tstests/unit/answer-ready-explain-pack.test.tstests/unit/cli.test.tstests/unit/context-pack-command.test.tstests/unit/context-prompt-command.test.tstests/unit/freshness-surfaces.test.tstests/unit/handoff-command.test.tstests/unit/stdio-server.test.tstests/unit/stdio-tool-profile.test.tstests/unit/why-madar-doc.test.ts
Address the current PR blockers by making prompt graph strictness fail fast, aligning handoff types, hardening freshness selection, making freshness assertions Windows-safe, and applying the small CodeRabbit cleanup fixes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Applied the remaining still-valid CodeRabbit follow-ups in b9cd20a. Included in the same fix set:
|
|
I would hold this PR before merge. The feature is directionally useful, but I don't think this should merge yet. It adds a broad freshness surface, but the core freshness model still needs to be tightened so we do not ship a noisy contract that has to be unwound later.
The issue is about turning staleness into a scoped diff. Current implementation compares every indexed source file mtime to the graph file mtime (
The test named "modified shared files on the selected route" changes Until these are resolved, this looks like a large additive surface around a freshness model that is still too easy to misclassify. I would prefer this PR first consolidate the freshness source-of-truth and contract shape, then layer CLI/MCP/docs on top. |
1 participant
Summary
--require-fresh-context/require_fresh_context), keep global strict mode, and reject unsupported review-task usage explicitlyVerification
Closes #477
Summary by CodeRabbit
New Features
Documentation