instrument(parser): establish strategy/work receipt foundation (#9444) - #7332
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
EffortlessSteven
left a comment
There was a problem hiding this comment.
Review verdict: not ready as work/benchmark authority. The receipt vocabulary separates the right categories, but several populated values are inferred constants or final-state sizes rather than observations of performed work. That is more dangerous than leaving them unknown because #7099 and claim cleanup will treat the receipt as ground truth. Instrument the actual construction, cloning, checkpoint-selection, and validation sites—or make unsupported fields explicitly unavailable—before publication.
| pub(crate) fn refresh_parse_output(&mut self) -> ParserInvocationReceipt { | ||
| let mut parser = Parser::new(self.source()); | ||
| let parse_output = parser.parse_with_recovery(); | ||
| let nodes_constructed = parse_output.ast.count_nodes(); |
There was a problem hiding this comment.
[P1] ast.count_nodes() measures the size of the final retained tree, not how many nodes the parser constructed. Recovery/backtracking may construct and discard additional nodes, so this value cannot support a performed-work claim. The next line also clones the full AST into the compatibility mirror, while the receipt later reports nodes_cloned = 0. Instrument construction and publication cloning at their actual sites, or rename these to final-tree size and make clone work explicit/unknown.
| nodes_patched: 0, | ||
| nodes_compared_only: 0, | ||
| checkpoints_restored, | ||
| checkpoints_invalidated: 0, |
There was a problem hiding this comment.
[P1] This field is documented as rejected or invalidated checkpoint candidates but is hard-coded to zero. The selection loop can reject multiple stored checkpoints for source identity, timeout sensitivity, edit overlap, or restore validity before choosing an earlier one or falling back. Count those decisions with stable reason tokens and carry them into the receipt; otherwise the fallback/eligibility envelope is systematically understated.
| 0, | ||
| state.source().len(), | ||
| state.tokens().len(), | ||
| state.parse_output().ast.count_nodes(), |
There was a problem hiding this comment.
[P1] The Unchanged path walks the entire AST to compute final_node_count, and then clones ParseOutput for the returned result, yet the receipt says zero analysis/validation work and zero cloned nodes. A no-op edit should either reuse stored counts and return an identity-backed snapshot, or account for this traversal and clone. As written, the supposedly zero-work path can be O(AST size) while reporting no work.
|
Read-only stale-PR preparation (live base fdfe5f1): disposition PREREQUISITE_BOUND. The receipt concept remains relevant, but required parser authorities and consumers are not all landed; the current head also retains challenged zero/inferred measurements. Reconcile prerequisites and split the receipt ownership before rebuilding this slice. No mutation was made. |
|
Disposition (campaign #11869 stale-queue sweep, 2026-08-21):
Idle >3h → reclaimable per umbrella #11869 queue policy. |
|
Disposition (campaign #11869 stale-queue sweep, 2026-08-22): |
Train position
Parser implementation train: #8700 — work-receipt W01
Controlling issue: #9444
Parent receipt controller: #7072
Restack ruling
This draft currently sits on the historical #6982 stored-checkpoint branch. Preserve its useful receipt work, but restack the W01 semantic slice off that dependency after #7292 and the live parser-work authority #8757/#8786 are current.
The restacked PR must not include #6982 checkpoint implementation merely because it is in this branch’s ancestry. Checkpoint-specific work moves to #7758. This PR is the shared receipt/invocation foundation needed by conservative
fresh_full_parseas well as later incremental strategies.Claim
Canonical parser operations carry one versioned per-strategy work receipt that records real parser invocations and authoritative live parser-work fields, with every other strategy-specific plane explicitly
not_applicable, partial, or not yet instrumented rather than fabricated as zero.Current correction
Review of the first candidate found false precision:
nodes_constructed;The restack must remove those claims. #7757 completes AST work; #7758 completes checkpoint/no-op/observation work.
Intended changes
ParseSnapshotwithout another source/config identity.Boundary
This PR does not implement:
A
fresh_full_parsereceipt may be complete while checkpoint fields are non-applicable. That is required so parser reliability does not wait on incremental checkpoint work.Validation
Use current equivalents of:
Closes #9444
Refs #7072
Refs #7757
Refs #7758
Refs #8171
Refs #7045