spec: define safe tool-call tail repair - #252
Conversation
🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 @.kiro/specs/safe-tool-call-tail-repair/design-review.md:
- Around line 165-175: Revise the Final Assessment and Implementation Gate in
the design review to scope the GO decision to design readiness, explicitly state
that implementation verification is pending, and avoid implying that
implementation is approved while spec.json approvals and tasks remain
incomplete. Clarify in the gate whether approvals.tasks.approved is required
before implementation begins, keeping the existing requirements.md and design.md
approval conditions explicit.
In @.kiro/specs/safe-tool-call-tail-repair/design.md:
- Around line 278-279: The post-syntax repair rule is ambiguous about byte
preservation and repair precedence. Update the design around the
completed-candidate rule to define whether preservation applies to the private
syntax candidate or published ArgsJSON, prohibit schema repairs from deleting or
appending bytes outside the allowed candidate boundary, and specify
deterministic reason-code precedence when syntax and schema repairs combine.
In @.kiro/specs/safe-tool-call-tail-repair/requirements.md:
- Around line 62-71: Update Requirement 3.3 in the safe-tool-call tail-repair
specification to describe an explicit default as an allowed policy-selected
deterministic fill, not as evidence that the value is the only permitted one.
Align the wording with the existing deterministic-fill boundary while retaining
const and single-valued enum as schema-constrained sources.
- Around line 110-117: Resolve the inconsistency between requirement 5.8 and the
empty-schema early return in the tool-call repair engine. Update the
requirements and design references, or change the empty-schema path around
isEmptySchema and syntaxOnlyOutcome, so every emitted rewrite either undergoes
compiled-schema validation or is covered by an explicit documented exception.
- Around line 89-98: Update the requirements around the classifier and preflight
flow so duplicate-member inputs may be provisionally classified, but must be
rejected by preflightArgsJSON before candidate construction results in any
published rewrite. Align requirement 4.6 with the existing duplicate rejection
behavior described in gap-analysis.md and design.md, preserving the rule that no
repair plan or rewrite is emitted for duplicate members.
In @.kiro/specs/safe-tool-call-tail-repair/research.md:
- Around line 211-219: Update the recommended order in the safe-tool-call repair
flow so cancellation and byte-size checks occur before resolving the tool. Keep
the existing sequence afterward, with resolve tool following those bounds checks
and preceding JSON validation and repair candidate construction.
In @.kiro/specs/safe-tool-call-tail-repair/tasks.md:
- Line 92: Update the validation commands in the task entries at lines 92 and
139 to use the intended `internal/core/toolcallrepair` package path by removing
the trailing underscore, while preserving the existing test and fuzz options.
- Around line 118-123: Update task 3.3 to define an explicit reason-code matrix
covering pending-value repair, existing schema repair, and their combined path,
including precedence rules for each outcome. Identify the expected codes in the
task and bind the corresponding fixtures in engine_contract_test.go to those
expectations so Res.ReasonCode is an explicit observable contract rather than
relying on unspecified engine semantics.
- Around line 197-201: Update task 5.2’s observable completion criterion to use
an explicit benchstat-based regression threshold for the valid fast path and
bounded near-limit work, matching the controlled benchmark requirements in the
design review. Remove “millisecond-scale” as a pass/fail gate; retain elapsed
time only as recorded benchmark evidence, with inputs, platform, percentile, and
acceptable regression limit defined.
- Around line 27-30: Update task 1.3’s invariant scope to call this
“completed-scalar recognition,” not scalar recovery; explicitly require refusal
assertions for incomplete numbers and literals, while preserving recognition of
already-completed scalars. Do not add scanner behavior that synthesizes partial
scalar values, and keep the existing append-only CompleteJSONSuffix tests
unchanged.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 7a2367d3-9958-45ec-be92-d8eaba6be03e
📒 Files selected for processing (7)
.kiro/specs/safe-tool-call-tail-repair/design-review.md.kiro/specs/safe-tool-call-tail-repair/design.md.kiro/specs/safe-tool-call-tail-repair/gap-analysis.md.kiro/specs/safe-tool-call-tail-repair/requirements.md.kiro/specs/safe-tool-call-tail-repair/research.md.kiro/specs/safe-tool-call-tail-repair/spec.json.kiro/specs/safe-tool-call-tail-repair/tasks.md
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Test (windows-latest)
- GitHub Check: Analyze (Go)
- GitHub Check: qa
🧰 Additional context used
🪛 LanguageTool
.kiro/specs/safe-tool-call-tail-repair/research.md
[style] ~47-~47: Consider replacing this word to strengthen your wording.
Context: ...ion failure. Go-LIP is a protocol proxy and does not own the agent's retry loop. *...
(AND_THAT)
[style] ~84-~84: Consider removing “of” to be more concise
Context: ...y. The transformation can be safe when all of the following hold: 1. the comma is the fi...
(ALL_OF_THE)
[grammar] ~284-~284: Ensure spelling is correct
Context: ...e:** fixtures, fuzz, race, conformance, dogfood, benchmarks, ADR.
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
.kiro/specs/safe-tool-call-tail-repair/requirements.md
[style] ~136-~136: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...dversarial tests shall cover very deep, very wide, oversized, invalid UTF-8, duplicate-ke...
(EN_WEAK_ADJECTIVE)
[grammar] ~152-~152: Ensure spelling is correct
Context: ...nt final tool-call semantics. 7.9 Local dogfood shall include deterministic examples fo...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (7)
.kiro/specs/safe-tool-call-tail-repair/research.md (1)
1-210: LGTM!Also applies to: 220-285
.kiro/specs/safe-tool-call-tail-repair/gap-analysis.md (1)
1-293: LGTM!.kiro/specs/safe-tool-call-tail-repair/requirements.md (1)
1-42: LGTM!Also applies to: 72-88, 99-109, 118-175
.kiro/specs/safe-tool-call-tail-repair/design.md (1)
1-277: LGTM!Also applies to: 280-392
.kiro/specs/safe-tool-call-tail-repair/design-review.md (1)
1-164: LGTM!.kiro/specs/safe-tool-call-tail-repair/tasks.md (1)
31-91: LGTM!Also applies to: 93-117, 124-138, 140-196, 202-228
.kiro/specs/safe-tool-call-tail-repair/spec.json (1)
1-23: LGTM!
9c2656f to
5c8d269
Compare
Summary
Adds a complete Kiro-style brownfield SDD specification for extending Go-LIP's automated native tool-call repair with the highest-value safe terminal-tail cases.
Included repair scope
const, a one-elementenum, ordefault.CompleteJSONSuffixappend-only and unchanged.Explicitly excluded
{}/[];null;Kiro artifacts
spec.jsonrequirements.mdgap-analysis.mdresearch.mddesign.mddesign-review.mdtasks.mdThe brownfield workflow includes current-asset mapping, 14 identified gaps, implementation-option analysis, requirements remediation, a three-round design validation ending in GO for design readiness, and a TDD-first execution plan. Implementation evidence remains pending and
ready_for_implementationremains false.Review remediation
CodeRabbit feedback was incorporated to:
ArgsJSON;benchstatcriterion; andGOdecision to design readiness and require all three human approvals plusready_for_implementationbefore implementation begins.Release-manifest hygiene
The repository's CI validates every PR commit independently. The original complete SDD commit and later review-remediation commits touch only paths explicitly approved through
.release-files.d/safe-tool-call-tail-repair.check-release-clean.shsupports exact-path fragments under.release-files.d/. This preserves the existing policy: every tracked path must still be explicitly approved, fragment paths must approve themselves, and stale entries are checked globally.Validation
mainat11e7a1f70116b6a86c08097aa45ab76f9de85f7b.kiro/specs/safe-tool-call-tail-repair/.release-files.d/safe-tool-call-tail-repair--refmodesready_for_implementation: falsepending maintainer approvalNo production Go code or product runtime behavior is changed in this PR.