Repository navigation
UN-4435 [FIX] Enable highlight automatically for HITL confidence rules - #2401
chandrasekharan-zipstack wants to merge 10 commits into
Conversation
When a workflow's HITL rules (DB or API) contain a confidence condition, the orchestrator marks the execution with auto_enable_highlight in manual_review_config and writes one line to the execution logs. The file worker then passes the structure tool a copy of the tool instance metadata with enable_highlight on. The saved setting and word confidence are not changed. The check comes from the manual_review plugin; the OSS null util returns False, so OSS behaviour is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CeVWyH8qfPJXCthxkH3Jc
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
Self-review round 1 — tip d1bbec1258fa0056433f4fa62725e9b6cf38c094
Review: unstract:review-deep (review-shallow refused: auth/tenancy trigger auth_authz_secrets, fired by organization_id= pass-through and a test mock named _get_platform_service_api_key). Mode: INITIAL. Diff 33224a79..d1bbec1, 5 files / 118 lines, base fetched fresh.
Verdict — REQUEST CHANGES
Summary — Critical: 0 · High: 2 · Medium: 3 · Low: 0 · Lenses run: 19/19
Signals: lens 4 raised (organization_id pass-through; no auth logic changed), lens 13 raised (tests changed).
Findings
Posted inline (5).
Unanchored findings
- PR title follows
TICKET [TYPE] text. Nothing to flag.
Lens checklist
- Spec & intent — See findings #1, #5
- Architectural fit & precedent — Clean
- Correctness & edge cases — See findings #1, #4, #5
- Security — Clean (orchestrator-assessed; no authz/tenant logic changed, organization_id only forwarded to existing log helper)
- Data integrity & migrations — N/A — no persisted state; override applied to a copy, never written
- Concurrency — N/A — no shared state or ordering changes
- API & contract compatibility — Clean (orchestrator-assessed; additive
manual_review_configkey, consumer defaults to False, rolling deploy safe both ways) - Reliability & resilience — Clean (orchestrator-assessed; two internal API calls via existing client, plugin catches per-type failures)
- Performance & cost — See findings #4
- Observability — Clean
- Operational safety — Clean (orchestrator-assessed; rollout risk captured in #1)
- LLM/agent-specific — See findings #5
- Testing — See findings #2, #3
- Dependencies & build — N/A — no dependency changes
- Code quality — Clean (ruff check/format at repo config: only I001 in
tests/test_structure_tool_pipeline.py:10, pre-existing on base, not this diff's lines) - Doc & comment accuracy — Clean (orchestrator-assessed)
- Cross-document rule consistency — Clean (merge sweep: 4 hits, all the new symbol's own definitions/uses)
- On-prem deployment experience — N/A — no chart, on-prem config or docs touched
- Scaling & workload lifecycle — N/A — no worker scaling config touched
boundary-sweep: 2 sinks traced, clean (two UI log lines carry a constant message). rule-storage-check: not covered — no markdown in the diff. Security deep-dive: done (no findings at confidence >= 8).
Open questions for the author — #4 and #5 above.
Assumptions made — Release order of OSS vs cloud is not enforced.
cost: agents=3 wall=05:10 lenses-examined=14 files=5 lines=118
…e flag path Self-review round 1: - Call has_confidence_rules through a getattr-guarded helper so an older manual_review plugin without the method does not fail every execution. - Document the accepted DB+API rule scan and the agentic no-confidence case. - Test the flag from manual_review_config to the structure tool call, the helper, and both orchestrators with rules present and absent. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CeVWyH8qfPJXCthxkH3Jc
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
Self-review round 2 — tip 5cfbe0661face00ea002543ceea902582ce1431f
Review: unstract:review-deep (review-shallow gate still refuses PR-wide: auth_authz_secrets). Mode: FOLLOWUP. Round delta d1bbec1..5cfbe06.
Verdict — APPROVE WITH NITS
Summary — Critical: 0 · High: 0 · Medium: 0 · Low: 1 · Lenses run: 19/19
Prior context fetched — followup-fetch.sh exited 0. Since-boundary d1bbec1258fa0056433f4fa62725e9b6cf38c094.
Scope change — NO
Prior findings status
- #4403419712 [High] Plugin without
has_confidence_rulesfails every run →RESOLVED— guarded helperworkers/shared/utils/manual_review_factory.py:393-401, both callers use it (workers/general/tasks.py:986,workers/api-deployment/tasks.py:674-676), older-plugin case tested. - #4403419723 [High] Flag path untested →
RESOLVED—test_auto_enable_highlight_read_from_review_configand parametrisedtest_structure_tool_params_passthrough. - #4403419734 [Medium] Orchestrators tested only with rules absent →
RESOLVED—workers/tests/test_barrier.pyTestAutoEnableHighlightOrchestration, both orchestrators, True/False, API with 2 batches. - #4403419740 [Medium] DB+API scan regardless of flow →
WAIVED— accepted in writing atworkers/shared/utils/manual_review_factory.py:394-398and in the PR body; matches the agreed scope ("check both DB and API rules"). - #4403419750 [Medium] Agentic forced highlight without confidence →
WAIVED— accepted in writing atworkers/shared/workflow/execution/service.py:1049-1051and the PR body caveat; decision surfaced to the task owner.
New findings
- [Low] [Lens 13] — General-orchestrator test's try/except comment gives the wrong reason
- Location:
workers/tests/test_barrier.py:L869-L885 - Failure mode: the
_create_batch_data_generalstub returnsNone, so the orchestrator raisesAttributeErroratgeneral/tasks.py:1038, before the chord. The comment says post-chord status updates raise; the chord patch is never reached. Assertions still pin the flag and the log, which happen first. - Suggested fix: stub returns
MagicMock(files=[])and records the config; correct the comment. - Confidence: High
- Verification: VERIFIED — ran the test with
--capture=no, saw theAttributeError.
- Location:
Lens checklist
- Spec & intent — Clean
- Architectural fit & precedent — Clean
- Correctness & edge cases — Clean
- Security — Clean (orchestrator-assessed; deep-dive done, 0 findings)
- Data integrity & migrations — N/A — no persisted state
- Concurrency — N/A — no shared state or ordering changes
- API & contract compatibility — Clean (orchestrator-assessed; delta adds no payload change)
- Reliability & resilience — Clean (orchestrator-assessed; helper fails closed to False)
- Performance & cost — Clean
- Observability — Clean
- Operational safety — Clean (orchestrator-assessed; deploy-order risk removed)
- LLM/agent-specific — Clean (agentic caveat documented)
- Testing — See findings #1
- Dependencies & build — N/A — no dependency changes
- Code quality — Clean (ruff format/check on changed lines clean; I001/D209 in test files pre-existing on base)
- Doc & comment accuracy — See findings #1
- Cross-document rule consistency — Clean (merge sweep hits are the new symbol's own uses)
- On-prem deployment experience — N/A — no on-prem surface touched
- Scaling & workload lifecycle — N/A — no scaling config touched
boundary-sweep: 0 new sinks in delta, clean. rule-storage-check: not covered — no markdown in the diff. Security deep-dive: done (no findings at confidence >= 8).
Open questions / assumptions — none new.
cost: agents=3 wall=03:09 lenses-examined=14 files=6 lines=311
The batch-data stub returned None, so the orchestrator raised at batch_data.files and a try/except with a wrong comment hid it. The stub now returns a batch with no files and the try/except is gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CeVWyH8qfPJXCthxkH3Jc
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
Self-review round 3 — tip a5c76fe4d50cf6bda1c2154ccc9b78d8696c892e
Review: unstract:review-deep (review-shallow gate still refuses PR-wide: auth_authz_secrets). Mode: FOLLOWUP. Round delta 5cfbe06..a5c76fe (workers/tests/test_barrier.py only).
Verdict — APPROVE
Summary — Critical: 0 · High: 0 · Medium: 0 · Low: 0 · Lenses run: 19/19
Prior context fetched — followup-fetch.sh exited 0. Since-boundary 5cfbe0661face00ea002543ceea902582ce1431f.
Scope change — NO
Prior findings status
- #4403419712 [High] →
RESOLVED(thread resolved in round 2) - #4403419723 [High] →
RESOLVED(thread resolved in round 2) - #4403419734 [Medium] →
RESOLVED(thread resolved in round 2) - #4403419740 [Medium] →
WAIVED— accepted in writing in code and PR body - #4403419750 [Medium] →
WAIVED— accepted in writing; agentic decision with the task owner, thread left open - Round 2 Low (General-orchestrator test try/except) →
RESOLVEDina5c76fe— stub returnsMagicMock(files=[])atworkers/tests/test_barrier.py:857-859, try/except removed; the orchestrator now returnsstatus="orchestrated".
New findings
No findings. Test agent mutated a scratch copy: removing the flag write, changing the log message, moving the flag write after the batch loop, or making the gate unconditional each fail the test.
Lens checklist
- Spec & intent — Clean
- Architectural fit & precedent — Clean
- Correctness & edge cases — Clean
- Security — Clean (deep-dive done, 0 findings)
- Data integrity & migrations — N/A — test-only delta
- Concurrency — N/A — test-only delta
- API & contract compatibility — N/A — test-only delta
- Reliability & resilience — N/A — test-only delta
- Performance & cost — N/A — test-only delta
- Observability — N/A — test-only delta
- Operational safety — N/A — test-only delta
- LLM/agent-specific — N/A — test-only delta
- Testing — Clean
- Dependencies & build — N/A — none changed
- Code quality — Clean (
ruff@0.8.4format/check: no hunks in the changed block) - Doc & comment accuracy — Clean (stale comment removed with its try/except)
- Cross-document rule consistency — Clean (no rule changed)
- On-prem deployment experience — N/A — no on-prem surface
- Scaling & workload lifecycle — N/A — none touched
boundary-sweep: 0 sinks in delta, clean. rule-storage-check: not covered — no markdown in the diff. Security deep-dive: done (no findings at confidence >= 8).
Open questions / assumptions — none new.
cost: agents=2 wall=03:46 lenses-examined=8 files=1 lines=39
- ETL/task runs check the DB rules only; API deployments check the API rules only. These are the rules each flow applies. - The override moves from the workflow service into structure_tool_task, after the agentic branch. Agentic runs keep the user's own setting: workflow runs give them no highlight data, so confidence cannot work. - The "highlight enabled automatically" message is now logged per file, only when the override applies. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CeVWyH8qfPJXCthxkH3Jc
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
Self-review round 4 — tip b829d3c961c7781f340db491728b0440bd3dc5fa
Review: unstract:review-deep (review-shallow gate still refuses PR-wide: auth_authz_secrets). Mode: FOLLOWUP. Round delta a5c76fe..b829d3c (7 files: per-flow rule type, override moved into the structure tool, agentic skipped).
Verdict — REQUEST CHANGES
Summary — Critical: 0 · High: 0 · Medium: 4 · Low: 0 · Lenses run: 19/19
Prior context fetched — followup-fetch.sh exited 0. Since-boundary a5c76fe4d50cf6bda1c2154ccc9b78d8696c892e.
Scope change — NO
Prior findings status
- #4403419712 [High] →
RESOLVED(thread resolved in round 2) - #4403419723 [High] →
RESOLVED(thread resolved in round 2) - #4403419734 [Medium] →
RESOLVED(thread resolved in round 2) - #4403419740 [Medium] →
WAIVED— accepted in writing in code and PR body - #4403419750 [Medium] →
RESOLVEDinb829d3c— agentic runs keep the user's setting; thread resolved
New findings
- [Medium] [Lens 3, 12] — API orchestrator checks API rules on calls whose destination never evaluates them, and misses the DB rules the file-history path does evaluate —
workers/api-deployment/tasks.py:673. The destination evaluates API rules only whenhitl_queue_nameis set;hitl_packet_idbypasses rules; with neither, it skips HITL unlessuse_file_history, where it evaluates DB rules (workers/shared/workflow/destination_connector.py:1580-1681). So plain API calls pay for highlight that nothing reads, and the file-history path can still score 100. Flagged by the cold read and the reliability/LLM agent. - [Medium] [Lens 10] — The auto-enable log line also fires where highlight cannot produce confidence —
workers/file_processing/structure_tool_task.py:375. Non-LLMWhisperer extractors, a missing highlight plugin, or summarize-as-source give no confidence data, so rules still score 100. The PR body notes this; the code does not. - [Medium] [Lens 7, 9] — Highlight is forced for every file in the run, which also adds the highlight postamble to prompts and highlight/confidence keys to metadata —
workers/general/tasks.py:985. By design: rules are evaluated after extraction, so every file needs confidence. Owed: say it in code. - [Medium] [Lens 3] — Agentic runs drop the flag with no log line —
workers/file_processing/structure_tool_task.py:372. Accepted: the task owner decided agentic runs get no log line; the comment at this line records the skip.
Lens checklist
- Spec & intent — See findings
- Architectural fit & precedent — Clean
- Correctness & edge cases — See findings
- Security — Clean
- Data integrity & migrations — N/A — no schema or data writes
- Concurrency — N/A — no shared state or ordering changed
- API & contract compatibility — See findings
- Reliability & resilience — Clean
- Performance & cost — See findings
- Observability — See findings
- Operational safety — N/A — no infra or rollout config
- LLM/agent-specific — See findings
- Testing — Clean
- Dependencies & build — N/A — none changed
- Code quality — Clean
- Doc & comment accuracy — Clean
- Cross-document rule consistency — Clean
- On-prem deployment experience — N/A — no on-prem surface
- Scaling & workload lifecycle — N/A — none touched
lens17-merge-sweep: exit 1, every hit in the branch's own files with consistent call shapes. boundary-sweep: 1 sink traced (fixed-string stream_log), clean. rule-storage-check: not covered — no markdown in the diff. Security deep-dive: done (no findings at confidence >= 8).
Open questions / assumptions — API file-history path: should the API orchestrator also check DB rules there, mirroring the destination? Raised with the task owner.
cost: agents=7 lenses-examined=19 files=7 lines=406
The destination evaluates API rules only when the request carries a hitl_queue_name, and a hitl_packet_id skips rule evaluation. Highlight on any other API call was cost nothing would read. Record in code the known limits: run-wide highlight, no confidence from non-LLMWhisperer V2 extractors or summarize-as-source, and the API + file-history path that evaluates DB rules. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CeVWyH8qfPJXCthxkH3Jc
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
Self-review round 5 — tip ad56d13e7e500b3a2a8f438d7a0ecda0d16d9bf3
Review: unstract:review-deep (review-shallow gate still refuses PR-wide: auth_authz_secrets). Mode: FOLLOWUP. Round delta b829d3c..ad56d13 (API rule check gated on hitl_queue_name / hitl_packet_id; known limits recorded in comments).
Verdict — REQUEST CHANGES
Summary — Critical: 0 · High: 0 · Medium: 2 · Low: 3 · Lenses run: 19/19
Prior context fetched — followup-fetch.sh exited 0. Since-boundary b829d3c961c7781f340db491728b0440bd3dc5fa.
Scope change — NO
Prior findings status
- #4403419712, #4403419723, #4403419734 →
RESOLVED(round 2) - #4403419740 →
WAIVED— accepted in writing in code and PR body - #4403419750 →
RESOLVEDinb829d3c - #4404194298 [Medium] API rule type →
PARTIALLY RESOLVEDinad56d13— queue-name / packet gates added; API + file-history DB path open, pending a decision - #4404194312 [Medium] log fires without confidence →
RESOLVEDinad56d13 - #4404194323 [Medium] run-wide highlight →
RESOLVEDinad56d13 - #4404194328 [Medium] agentic no log →
WAIVED— task owner decision
New findings
- [Medium] [Lens 7] — Forced highlight adds
confidence_data,line_numbers,whisper_hash(and, with the org flag on,highlight_data) to the API response metadata forinclude_metadatacallers whose tool had highlight off —workers/file_processing/structure_tool_task.py:378. The response path strips onlyhighlight_data, and only on the org flag (backend/api_v2/deployment_helper.py:402-411). Additive keys; whether to strip them is a design decision. - [Medium] [Lens 2, 3] — The orchestrator copies the destination's rule-type choice, and the copy misses the API + file-history DB branch —
workers/api-deployment/tasks.py:673. Same gap as #4404194298; the cold read suggests one sharedhitl_rule_type(...)helper used by_should_handle_hitland both orchestrators. - [Low] [Lens 16] — "Each file gets the highlight prompt postamble" overstates: agentic, summarize-as-source and non-LLMWhisperer V2 runs get neither —
workers/general/tasks.py:985. - [Low] [Lens 16] — Test docstring leaves out the
hitl_packet_idexclusion —workers/tests/test_barrier.py:779. - [Low] [Lens 15] — Leftover local
tool_instance_metadatapassed through unchanged —workers/shared/workflow/execution/service.py:1047.
Lens checklist
- Spec & intent — Clean
- Architectural fit & precedent — See findings
- Correctness & edge cases — See findings
- Security — Clean
- Data integrity & migrations — N/A — no schema or data writes
- Concurrency — N/A — no shared state or ordering changed
- API & contract compatibility — See findings
- Reliability & resilience — Clean
- Performance & cost — Clean
- Observability — Clean
- Operational safety — N/A — no infra or rollout config
- LLM/agent-specific — Clean
- Testing — Clean
- Dependencies & build — N/A — none changed
- Code quality — See findings
- Doc & comment accuracy — See findings
- Cross-document rule consistency — Clean
- On-prem deployment experience — N/A — no on-prem surface
- Scaling & workload lifecycle — N/A — none touched
lens17: no rule text changed beyond the comments reviewed under lens 16. boundary-sweep: 0 new sinks in delta, clean. rule-storage-check: not covered — no markdown in the diff. Security deep-dive: done (no findings at confidence >= 8).
Open questions / assumptions — Both Mediums need the task owner's call: strip forced keys from the API response, and cover the API + file-history DB path (shared helper). Until then they are recorded in code and the PR body.
cost: agents=6 lenses-examined=19 files=7 lines=432
Narrow the run-wide comment to what holds for every file, name the hitl_packet_id exclusion in the test docstring, record that forced highlight adds metadata keys to include_metadata API responses, and pass tool_instance.metadata inline again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CeVWyH8qfPJXCthxkH3Jc
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
Review — FOLLOWUP (self-review round 6)
Tip: b0e2218 · delta ad56d13..b0e2218 · review: review-deep (whole PR touches an API payload path, so lite is not eligible).
Verdict — REQUEST CHANGES
Summary — Critical: 0 · High: 1 · Medium: 0 · Low: 1
Prior context fetched
— followup-fetch.sh exited 0; since boundary ad56d13.
Scope change
— NO
Prior findings status
- API rule type on the API + file-history path [Medium] → PARTIALLY RESOLVED — gate now limited to
hitl_queue_namerequests (workers/api-deployment/tasks.py:673-685); the file-history path still checks no DB rules. Recorded in code; pending product decision. - API response metadata gains highlight keys [Medium] → NOT RESOLVED — pending decision (strip or document).
- Duplicated rule-type routing [Medium] → NOT RESOLVED — pending decision on a shared helper.
- Three Lows from the previous round → RESOLVED in b0e2218.
New findings
- [High] Forced highlight changes extraction, not only metadata —
workers/file_processing/structure_tool_task.py:372-380,workers/api-deployment/tasks.py:673-678. With highlight on,answer_prompt.py:120-126keeps the platform postamble (line-number instruction) in every prompt; LLMWhisperer V2 is called withadd_line_nos/include_line_confidenceand makes an extra highlight request (unstract/sdk1/.../llm_whisperer_v2/src/helper.py:271-272, 338-344); the highlight plugin post-processes answers (legacy_executor.py:1409-1427). Comments, the log line and the PR body describe the override as only adding confidence data. Failure: every file in a run with a confidence rule gets a different prompt and extracted text, so answers can change for users who never enabled highlight. Mechanism verified; answer-level change not reproduced. Fix: product decision — get confidence without the prompt postamble, or accept and document that prompts and extracted text change. - [Low] Metadata key wording —
workers/api-deployment/tasks.py:677-678and PR body: say highlight addsline_numbersandwhisper_hash.answer_prompt.py:308-317always writeshighlight_data,line_numbersandwhisper_hash; onlyconfidence_datais new, the others get filled.
Security deep-dive: done (0 findings).
Lens checklist
- 1: See findings #2
- 2: Clean
- 3: See findings #1
- 4: Clean
- 5: N/A — no schema or data migration
- 6: Clean
- 7: Clean — API metadata change tracked in prior findings
- 8: Clean
- 9: Clean
- 10: Clean
- 11: N/A — no infra or rollout config
- 12: See findings #1
- 13: Clean
- 14: N/A — no dependency change
- 15: Clean
- 16: See findings #1, #2
- 17: Clean
- 18: N/A — no on-prem surface
- 19: N/A — no scaling or workload config
Open questions / assumptions
— Assumes answers can change with the postamble; not reproduced end to end yet (dev-namespace test pending).
Correct the metadata-key wording and drop ticket IDs from test docstrings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CeVWyH8qfPJXCthxkH3Jc
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
Review — FOLLOWUP (self-review round 7)
Tip: 49e7b43 · delta b0e2218..49e7b43 · review: review-deep (lite refused: security disqualifier over the whole PR).
Verdict — APPROVE WITH NITS
Summary — Critical: 0 · High: 0 · Medium: 0 · Low: 1
Prior context fetched
followup-fetch.sh exited 0; since boundary b0e2218.
Scope change
NO
Prior findings status
- Forced highlight changes extraction [High] → PARTIALLY RESOLVED — documented at workers/file_processing/structure_tool_task.py:378-380 and in the PR body; product decision still open.
- Metadata key wording [Low] → RESOLVED in 49e7b43.
- API + file-history DB rules [Medium], API response metadata [Medium], shared routing helper [Medium] → NOT RESOLVED — pending product decisions.
New findings
- [Low] workers/api-deployment/tasks.py:677-679 — comment says include_metadata callers receive highlight_data; backend/api_v2/deployment_helper.py:401-411 strips it unless ENABLE_HIGHLIGHT_API_DEPLOYMENT is on (default off). The PR body states it correctly. Also, the two comments describe behaviour but do not say a decision is pending, while the body says "recorded in code". Fix: drop highlight_data from the received list; mark both as pending decisions.
Security deep-dive: done (0 findings). Cold read: no findings. Tests: 54 passed (re-run by reviewer).
Lens checklist
- 1: Clean
- 2: Clean
- 3: Clean
- 4: Clean
- 5: N/A — no schema or data migration
- 6: Clean
- 7: Clean
- 8: Clean
- 9: Clean
- 10: Clean
- 11: N/A — no infra or rollout config
- 12: Clean
- 13: Clean
- 14: N/A — no dependency change
- 15: Clean
- 16: See findings #1
- 17: Clean
- 18: N/A — no on-prem surface
- 19: N/A — no scaling or workload config
Open questions / assumptions
Answer-level change from the postamble not reproduced yet; dev namespace now deployed (snapshot.529).
…ions Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CeVWyH8qfPJXCthxkH3Jc
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
Review — FOLLOWUP (self-review round 8)
Tip: b5ee69b · delta 49e7b43..b5ee69b · review: review-deep (lite refuses this PR on the security disqualifier).
Verdict — APPROVE
Summary — Critical: 0 · High: 0 · Medium: 0 · Low: 0
Prior context fetched
followup-fetch.sh exited 0; since boundary 49e7b43.
Scope change
NO
Prior findings status
- highlight_data exposure wording [Low] → RESOLVED in b5ee69b (deployment_helper.py:401-411, 611-620 strip it unless the org flag is on).
- Forced highlight changes extraction [High] → PARTIALLY RESOLVED — documented in code and PR body; product decision open.
- API + file-history DB rules, API response metadata, shared routing helper [Medium] → NOT RESOLVED — pending product decisions.
New findings
No findings at confidence >= 8. Comment check, cold read and security scan each returned none. Tests: 54 passed (re-run).
Security deep-dive: done (0 findings).
Lens checklist
- 1: Clean
- 2: Clean
- 3: Clean
- 4: Clean
- 5: N/A — no schema or data migration
- 6: Clean
- 7: Clean
- 8: Clean
- 9: Clean
- 10: Clean
- 11: N/A — no infra or rollout config
- 12: Clean
- 13: Clean
- 14: N/A — no dependency change
- 15: Clean
- 16: Clean
- 17: Clean
- 18: N/A — no on-prem surface
- 19: N/A — no scaling or workload config
Open questions / assumptions
Below threshold: the Docker (non-structure) tool path ignores the flag silently; files served from file history keep their earlier no-highlight result.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CeVWyH8qfPJXCthxkH3Jc
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
Review — FOLLOWUP (self-review round 9)
Tip: 8f67921 · delta b5ee69b..8f67921 · review: review-deep (lite refuses this PR on the security disqualifier).
Verdict — REQUEST CHANGES
Summary — Critical: 0 · High: 0 · Medium: 1 · Low: 0
Prior context fetched
followup-fetch.sh exited 0; since boundary b5ee69b.
Scope change
NO
Prior findings status
- API + use_file_history DB rules, API response metadata, shared routing helper, forced-highlight extraction change → WAIVED — PR author decisions, replies on each thread; recorded in code in 8f67921.
New findings
- [Medium] workers/api-deployment/tasks.py:675-677 and PR body "Known gap" bullet — comment says use_file_history is internal-only and to add a DB check "if it is ever exposed". It is a validated request field (backend/api_v2/serializers.py:317), only hidden from the OpenAPI schema (L280), read at api_deployment_views.py:120. An API caller can send it; the destination then evaluates DB rules (destination_connector.py:1643-1681) with no highlight. Fix: describe it as undocumented but caller-settable; keep the out-of-scope decision stated as a decision.
Security deep-dive: done (0 findings). Cold read: no findings. Tests: 54 passed (re-run).
Lens checklist
- 1: See findings #1
- 2: Clean
- 3: Clean
- 4: Clean
- 5: N/A — no schema or data migration
- 6: Clean
- 7: Clean
- 8: Clean
- 9: Clean
- 10: Clean
- 11: N/A — no infra or rollout config
- 12: Clean
- 13: Clean
- 14: N/A — no dependency change
- 15: Clean — ruff 0.8.4 format/check run on changed files
- 16: See findings #1
- 17: Clean
- 18: N/A — no on-prem surface
- 19: N/A — no scaling or workload config
Open questions / assumptions
Below threshold: one extra rule-engine call per cloud ETL execution (once per run); API path re-fetches API rules already fetched in _create_file_data.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CeVWyH8qfPJXCthxkH3Jc
|
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
Review — FOLLOWUP (self-review round 10)
Tip: a2a8ccd · delta 8f67921..a2a8ccd · review: review-deep (lite refuses this PR on the security disqualifier).
Verdict — APPROVE
Summary — Critical: 0 · High: 0 · Medium: 0 · Low: 0
Prior context fetched
followup-fetch.sh exited 0; since boundary 8f67921.
Scope change
NO
Prior findings status
- use_file_history described as internal-only [Medium] → RESOLVED in a2a8ccd (workers/api-deployment/tasks.py:675-678; matches backend/api_v2/serializers.py:282,317).
New findings
No findings at confidence >= 8. Comment check, cold read and security scan each returned none. Tests: 54 passed (re-run).
Security deep-dive: done (0 findings).
Lens checklist
- 1: Clean
- 2: Clean
- 3: Clean
- 4: Clean
- 5: N/A — no schema or data migration
- 6: Clean
- 7: Clean
- 8: Clean
- 9: Clean
- 10: Clean
- 11: N/A — no infra or rollout config
- 12: Clean
- 13: Clean
- 14: N/A — no dependency change
- 15: Clean — ruff 0.8.4 format/check run on changed files
- 16: Clean
- 17: Clean
- 18: N/A — no on-prem surface
- 19: N/A — no scaling or workload config
Open questions / assumptions
Not verified by this round: the cloud plugin scores a confidence rule as 100 when no confidence data exists (stated in structure_tool_task.py; from the original ticket analysis). End-to-end run in the dev namespace is in progress.
[Medium impact] The PR appears safe to merge within its stated scope.
|
Unstract test resultsPer-group results
Critical paths
|



What
hitl_queue_nameand nohitl_packet_id, the only API requests whose destination evaluates API rules).Why
enable_highlightcomes from the tool instance settings. When it is off, no confidence data is produced, and the HITL rule engine scores every field as 100. Thenconfidence <= Xrules never route a document to review, and>=rules always do. Nothing tells the user.How
general/tasks.py(ETL and task workflows) callshas_confidence_rules(workflow_util, workflow_id, "DB"), andapi-deployment/tasks.pycalls it with"API"when the request hashitl_queue_nameand nohitl_packet_id(helper inshared/utils/manual_review_factory.py). Other API requests skip the check: their destination evaluates no API rules. Each does this once per execution, before batching. If it returns True, it setsmanual_review_config["auto_enable_highlight"] = True.shared/workflow/execution/service.py: the file worker reads the flag and passes it to the structure tool in its params (auto_enable_highlight). The tool instance metadata is passed unchanged.file_processing/structure_tool_task.py: after the agentic branch returns, a set flag makes the tool use{**tool_instance_metadata, "enable_highlight": True}and log the line. This is a copy. The saved setting is never written back, so when the rules are removed the user's own setting applies again.line_metadata, sohighlight_datais always{}and confidence can't work there. Making confidence work for agentic projects is a separate ticket.enable_word_confidenceis not changed. The user opts in to it because its output-token cost grows with the number of fields.manual_reviewplugin (cloud PR below). The OSS helper returnsFalsewhen the util has nohas_confidence_rulesmethod: the OSS null util, or an older plugin built before the cloud PR.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
manual_reviewplugin): no change. The helper returnsFalse, so the flag is never set.manual_reviewplugin (nohas_confidence_rules): no change, same as OSS.include_metadata) now getconfidence_datain it, and theline_numbersandwhisper_hashkeys, which are always present, are now filled.highlight_datais filled too, but callers see it only when the org's API-deployment highlight flag is on. This happens even though the tool's highlight checkbox is off. The change is additive, and the keys are kept on purpose so callers can see why a document was routed.include_metadatamay now carryconfidence_data/line_numbers/whisper_hash/highlight_datawhen HITL confidence rules exist.use_file_historyand nohitl_queue_namehas DB rules evaluated at the destination, but the API orchestrator checks only API rules, so a DB confidence rule there still scores 100.use_file_historyis undocumented (hidden from the API schema, meant for demos), though a caller can still send it; this path is left out of scope for now and noted in code.Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
tests/test_structure_tool_pipeline.py:TestAutoEnableHighlight.test_regular_overrides_a_copy: user off + rule = on, user on + no rule = on, off + off = off. Word confidence and the saved metadata are unchanged. The log line is written only when the override applies.TestAutoEnableHighlight.test_agentic_keeps_user_setting: agentic + flag set + user highlight off = X2Text called withenable_highlight=False, and no log line.test_auto_enable_highlight_read_from_review_configandtest_auto_enable_highlight_passed_in_params: the flag travels frommanual_review_configto the structure tool params, and the metadata is unchanged.test_has_confidence_rules_helper: True, False, an older plugin without the method, and the rule type is passed through.tests/test_barrier.pyTestAutoEnableHighlightOrchestration: the API orchestrator sets the flag on every batch only for an API confidence rule on ahitl_queue_namerequest (a DB rule alone, no queue name, or ahitl_packet_iddoes not set it, and the rules are not fetched), and the general orchestrator only for a DB confidence rule (an API rule alone does not set it).workers/:uv run --group test pytest tests/test_structure_tool_pipeline.py tests/test_barrier.py -q --no-cov: 54 passed.hitl_queue_namegate removed, APIhitl_packet_idgate removed.Highlight enabled automatically..., the extractor's highlight parameters, the HITL rule evaluation) and the API response metadata.confidence.vendor_name != 100: the log line is written, LLMWhisperer runs with highlight and line numbers, and the destination evaluates the DB rule.output.vendor_name): no log line, highlight stays off.hitl_queue_name: the log line is written; the response metadata hasconfidence_dataand filledline_numbersandwhisper_hash(nohighlight_data, the org flag is off).hitl_queue_name: no log line, noconfidence_data, emptyline_numbersandwhisper_hash.enable_highlighton both tool instances is stillfalseafter the runs.Screenshots
Checklist
I have read and understood the Contribution Guidelines.
🤖 Generated with Claude Code