Add manifest evaluation resource budgets (#651) - #670
Conversation
Bound template rendering, expressions, macro expansion, and `foreach` processing with shared fuel, byte, source, cardinality, and expansion limits. Keep trusted operator ceilings authoritative while allowing project settings to narrow them, and expose bounded redacted diagnostics and telemetry. Document the policy and cover its limits across unit, property, query, BDD, localization, and configuration metadata tests.
|
Warning Review limit reachedNext included review available in 16 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 98 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (85)
Warning Your free Security trial is over. An organization admin can upgrade to Advanced for continuous pull request security review or dismiss this notice. Comment |
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message in a code block that I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Comment on lines +28 to +37 fn rendered_value_at_limit_succeeds() -> Result<()> {
let yaml = concat!(
"netsuke_version: 1.0.0\n",
"targets:\n",
" - name: exact\n",
" command: '{{ \"x\" * 16 }}'\n",
);
from_str_with_limits(yaml, small_limits())?;
Ok(())
}❌ New issue: Code Duplication |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message in a code block that I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. src/manifest/jinja_macros/mod.rs Comment on lines +289 to +300 ) -> Result<String, Error> {
let budget = ManifestBudget::default();
render_template_at(
env,
&budget,
&TemplateRenderRequest {
template,
context,
stage: ManifestBudgetStage::Render,
},
)
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message in a code block that I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Comment on lines +237 to +248 pub(crate) fn new(limits: ManifestBudgetLimits) -> Result<Self> {
let validated_limits = limits.validate()?;
Ok(Self {
state: Rc::new(ManifestBudgetState {
rendered_bytes: Cell::new(validated_limits.rendered_manifest_bytes),
source_bytes: Cell::new(validated_limits.source_bytes),
expanded_entries: Cell::new(validated_limits.expanded_entries),
fuel: Cell::new(validated_limits.manifest_fuel),
}),
limits: validated_limits,
})
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Delegate the test-only wrapper to the budget-aware rendering entry point so both paths construct the same request while retaining a fresh default budget.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @leynos, your pull request is larger than the review limit of 150,000 diff characters
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![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: d9b2dd34b3
ℹ️ 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".
| /// Maximum `MiniJinja` instructions for one manifest evaluation. | ||
| #[arg(long, value_name = "FUEL", default_value_t = 1_000_000)] | ||
| pub manifest_evaluation_fuel: u64, |
There was a problem hiding this comment.
When any --manifest-* option is supplied, Clap stores it in Cli, but cli_overrides_from_matches never inserts any of the seven new fields into the CLI merge layer; apply_config then replaces the parsed values with the merged defaults or file/environment values. Consequently an operator invocation such as --manifest-fuel 1000 silently runs with a much larger ceiling, defeating the command-line resource restriction.
Useful? React with 👍 / 👎.
| if is_project_scope_layer(path.as_deref(), project_key.as_deref()) { | ||
| project_budget_request = take_project_manifest_budget_request(&mut value); |
There was a problem hiding this comment.
When the project .netsuke.toml uses extends, only the layer whose path exactly matches .netsuke.toml has its budget fields extracted for monotonic reconciliation. An inherited file is still controlled by the project but remains a normal high-precedence file layer, so a checkout can put an enlarged manifest_fuel or byte ceiling in base.toml and extend it, bypassing the documented rule that project configuration cannot widen operator defaults.
Useful? React with 👍 / 👎.
| ManifestBudgetStage::Source, | ||
| ) | ||
| .map_err(|exhaustion| exhaustion.into_error(ErrorKind::WriteFailure))?; | ||
| register_macro(env, def, idx).with_context(|| { |
There was a problem hiding this comment.
When a manifest macro is called from a bare foreach or when expression, it uses the global-function fallback in invocation.rs, whose capture_macro(...).render_captured(()) creates a fresh MiniJinja state and materializes the macro result as a String. This registration path never gives that wrapper the ManifestBudget, so macro-body fuel is not charged to the caller's state and macro output bypasses both rendered-value and aggregate byte writers; a compact expression macro can therefore allocate large output or multiply instruction work despite the new limits.
Useful? React with 👍 / 👎.
| fields | ||
| .remove(name) | ||
| .and_then(|value| serde_json::from_value(value).ok()) |
There was a problem hiding this comment.
When a project budget key has the wrong TOML type or an out-of-range numeric representation, take_limit removes the key and discards the deserialization error through .ok(). The remaining layer then passes normal schema validation and Netsuke silently uses another ceiling instead of reporting the invalid configuration, making operator-visible configuration mistakes indistinguishable from an omitted restriction.
Useful? React with 👍 / 👎.
1 participant
Summary
when, and lazyforeachexpansion.Closes #651
References